Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

peaclock: init at 0.4.3 #101231

Merged
merged 2 commits into from Oct 28, 2020
Merged

peaclock: init at 0.4.3 #101231

merged 2 commits into from Oct 28, 2020

Conversation

djanatyn
Copy link
Contributor

Motivation for this change

I wanted to use this clock in my terminal. It's cute!

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@djanatyn djanatyn force-pushed the pkg/peaclock branch 2 times, most recently from cefffb8 to be75ae6 Compare October 21, 2020 03:12
@djanatyn
Copy link
Contributor Author

I tried to get this to build on OS X (since the repository says MacOS is supported) using gcc8Stdenv, but it didn't link successfully:

{ stdenv, lib, fetchFromGitHub, cmake, pkgconfig, libpthreadstubs, icu }:

stdenv.mkDerivation rec {
  pname = "peaclock";
  version = "0.4.3";

  src = fetchFromGitHub {
    owner = "octobanana";
    repo = pname;
    rev = version;
    sha256 = "1582vgslhpgbvcd7ipgf1d1razrvgpq1f93q069yr2bbk6xn8i16";
  };

  nativeBuildInputs = [ cmake ];
  buildInputs = [ libpthreadstubs icu ];

  cmakeFlags = [ "-DCMAKEBUILD_TYPE=release" ] ++ lib.optional (stdenv.isDarwin)
    "-DCMAKE_CXX_COMPILER=${stdenv.cc}/bin/c++";

  enableParallelBuilding = true;

  meta = with stdenv.lib; {
    description = "A clock, timer, and stopwatch for the terminal.";
    homepage = "https://octobanana.com/software/peaclock";
    license = licenses.mit;
    platforms = [ "x86_64-darwin" "x86_64-linux" "i686-linux" ];
    maintainers = with maintainers; [ djanatyn ];
  };
}
+  peaclock = callPackage ../applications/misc/peaclock {
+    stdenv = if stdenv.isDarwin then gcc8Stdenv else stdenv;
+  };

Build output:

[ 16%] Building CXX object CMakeFiles/peaclock.dir/src/main.cc.o
[ 50%] Building CXX object CMakeFiles/peaclock.dir/src/ob/string.cc.o
[ 50%] Building CXX object CMakeFiles/peaclock.dir/src/peaclock/tui.cc.o
[ 66%] Building CXX object CMakeFiles/peaclock.dir/src/ob/readline.cc.o
[ 83%] Building CXX object CMakeFiles/peaclock.dir/src/peaclock/peaclock.cc.o
[100%] Linking CXX executable peaclock
ld: warning: option -s is obsolete and being ignored
ld: warning: passed two min versions (10.12.0, 10.12) for platform macOS. Using 10.12.
Undefined symbols for architecture x86_64:
  "std::filesystem::__cxx11::path::has_filename() const", referenced from:
      std::filesystem::__cxx11::path::operator/=(std::filesystem::__cxx11::path const&) in main.cc.o
  "std::filesystem::__cxx11::path::lexically_normal() const", referenced from:
      Tui::mkconfig(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool) in tui.cc.o
  "std::filesystem::__cxx11::path::has_root_directory() const", referenced from:
      std::filesystem::__cxx11::path::operator/=(std::filesystem::__cxx11::path const&) in main.cc.o
  "std::filesystem::__cxx11::path::compare(std::filesystem::__cxx11::path const&) const", referenced from:
      _main in main.cc.o
      Tui::load_config(std::filesystem::__cxx11::path const&) in tui.cc.o
  "std::filesystem::create_directory(std::filesystem::__cxx11::path const&)", referenced from:
      _main in main.cc.o
  "std::filesystem::status(std::filesystem::__cxx11::path const&)", referenced from:
      _main in main.cc.o
      Tui::mkconfig(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool) in tui.cc.o
      Tui::load_config(std::filesystem::__cxx11::path const&) in tui.cc.o
  "std::filesystem::__cxx11::path::_M_split_cmpts()", referenced from:
      std::filesystem::__cxx11::path::path<char [5], std::filesystem::__cxx11::path>(char const (&) [5], std::filesystem::__cxx11::path::format) [clone .isra.274] in main.cc.o
      std::filesystem::__cxx11::path::operator/=(std::filesystem::__cxx11::path const&) in main.cc.o
      std::filesystem::__cxx11::path OB::Parg::get<std::filesystem::__cxx11::path, 0>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) in
 main.cc.o
      _main in main.cc.o
      Tui::mkconfig(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool) in tui.cc.o
      Tui::load_config(std::filesystem::__cxx11::path const&) in tui.cc.o
ld: symbol(s) not found for architecture x86_64
collect2: error: ld returned 1 exit status
make[2]: *** [CMakeFiles/peaclock.dir/build.make:163: peaclock] Error 1
make[1]: *** [CMakeFiles/Makefile2:95: CMakeFiles/peaclock.dir/all] Error 2
make: *** [Makefile:149: all] Error 2
builder for '/nix/store/g6wdj74dx7s5wjddz69v7z8782wdnjhy-peaclock-0.4.3.drv' failed with exit code 2
error: build of '/nix/store/g6wdj74dx7s5wjddz69v7z8782wdnjhy-peaclock-0.4.3.drv' failed

@djanatyn
Copy link
Contributor Author

I was able to build successfully on pinephone, so I added aarch64-linux support.

r-burns
r-burns requested changes Oct 22, 2020
View reviewed changes
Copy link
Contributor

@r-burns r-burns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hehe it is cute!

I was able to run on macos with these changes, and the LC_ALL=C workaround from octobanana/peaclock#16. It looks like gccStdenv is gcc 9, which has a fully functioning stdc++fs. Although clang 7 is supposed to work, so maybe that would be preferable?

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/peaclock/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/peaclock/default.nix Outdated Show resolved Hide resolved
@djanatyn
Copy link
Contributor Author

I tried stdenvClang:

[ 16%] Building CXX object CMakeFiles/peaclock.dir/src/ob/string.cc.o
[ 33%] Building CXX object CMakeFiles/peaclock.dir/src/peaclock/tui.cc.o
[ 50%] Building CXX object CMakeFiles/peaclock.dir/src/main.cc.o
[ 66%] Building CXX object CMakeFiles/peaclock.dir/src/ob/readline.cc.o
[ 83%] Building CXX object CMakeFiles/peaclock.dir/src/peaclock/peaclock.cc.o
[100%] Linking CXX executable peaclock
ld: warning: option -s is obsolete and being ignored
ld: warning: passed two min versions (10.12.0, 10.12) for platform macOS. Using 10.12.
ld: warning: ignoring file CMakeFiles/peaclock.dir/src/main.cc.o, building for macOS-x86_64 but attempting to link with file built for unknown-unsupported file format ( 0xDE 0x
C0 0x17 0x0B 0x00 0x00 0x00 0x00 0x14 0x00 0x00 0x00 0xF4 0x65 0x08 0x00 )
ld: warning: ignoring file CMakeFiles/peaclock.dir/src/ob/string.cc.o, building for macOS-x86_64 but attempting to link with file built for unknown-unsupported file format ( 0x
DE 0xC0 0x17 0x0B 0x00 0x00 0x00 0x00 0x14 0x00 0x00 0x00 0xD8 0xB6 0x01 0x00 )
ld: warning: ignoring file CMakeFiles/peaclock.dir/src/peaclock/tui.cc.o, building for macOS-x86_64 but attempting to link with file built for unknown-unsupported file format (
 0xDE 0xC0 0x17 0x0B 0x00 0x00 0x00 0x00 0x14 0x00 0x00 0x00 0x54 0x35 0x0E 0x00 )
ld: warning: ignoring file CMakeFiles/peaclock.dir/src/ob/readline.cc.o, building for macOS-x86_64 but attempting to link with file built for unknown-unsupported file format (
0xDE 0xC0 0x17 0x0B 0x00 0x00 0x00 0x00 0x14 0x00 0x00 0x00 0x80 0xB5 0x05 0x00 )
ld: warning: ignoring file CMakeFiles/peaclock.dir/src/peaclock/peaclock.cc.o, building for macOS-x86_64 but attempting to link with file built for unknown-unsupported file for
mat ( 0xDE 0xC0 0x17 0x0B 0x00 0x00 0x00 0x00 0x14 0x00 0x00 0x00 0x48 0x44 0x06 0x00 )
Undefined symbols for architecture x86_64:
  "_main", referenced from:
     implicit entry/start for main executable
ld: symbol(s) not found for architecture x86_64
clang-7: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [CMakeFiles/peaclock.dir/build.make:163: peaclock] Error 1
make[1]: *** [CMakeFiles/Makefile2:95: CMakeFiles/peaclock.dir/all] Error 2
make: *** [Makefile:149: all] Error 2
builder for '/nix/store/hkmlb9b88vm01mlvh86dl5f18g0x4zci-peaclock-0.4.3.drv' failed with exit code 2
error: build of '/nix/store/hkmlb9b88vm01mlvh86dl5f18g0x4zci-peaclock-0.4.3.drv' failed

No worries - your changes worked and I got peaclock running on my macbook 🎉 Thank you @r-burns!

I was looking at makeWrapper for setting LC_ALL=C on certain platforms. However, I think it might be best to leave that to be fixed in upstream given that it affects all musl distributions?

If so, I think this is working on all platforms based on my testing.

@AndersonTorres
Copy link
Member

Now squash the 5 commits to 2 only (the one for the new package maintainer and the other for the package itself).

IvarWithoutBones
IvarWithoutBones requested changes Oct 22, 2020
View reviewed changes
Copy link
Member

@IvarWithoutBones IvarWithoutBones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few suggestions :)

pkgs/applications/misc/peaclock/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/peaclock/default.nix Outdated Show resolved Hide resolved
@AndersonTorres
Copy link
Member

Also, squash the commits.

djanatyn and others added 2 commits October 27, 2020 10:04
@djanatyn
maintainers: add djanatyn
74504e8
@djanatyn @r-burns
peaclock: init at 0.4.3
c84ddc7 
Co-authored-by: r-burns <52847440+r-burns@users.noreply.github.com>
@djanatyn
Copy link
Contributor Author

Commits squashed, review comments resolved. Thanks for walking me through this, it's so exciting contributing to something I use everyday.

@AndersonTorres
Copy link
Member

LGTM, LGTBorg.

@AndersonTorres AndersonTorres merged commit c13da09 into NixOS:master Oct 28, 2020
league added a commit to ledatelescope/bifrost that referenced this pull request Apr 6, 2022
@league
nix: use gccStdenv for consistency across linux/darwin
10a2322 
Otherwise stdenv on darwin uses clang, and we had some trouble linking
the c++17 filesystem stuff there. (Maybe it's just a flag that's
missing, but I haven't figured it out yet.) Similar errors were
reported for a different nix package, and they resolved using
`gccStdenv` too: NixOS/nixpkgs#101231
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IvarWithoutBones IvarWithoutBones IvarWithoutBones requested changes

@r-burns r-burns r-burns requested changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@djanatyn @AndersonTorres @IvarWithoutBones @r-burns @cole-h