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.

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

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

@djanatyn djanatyn commented Oct 21, 2020

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
Copy link
Contributor Author

@djanatyn djanatyn commented Oct 21, 2020

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

@djanatyn djanatyn commented Oct 22, 2020

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

Copy link
Contributor

@r-burns r-burns left a comment

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

@djanatyn djanatyn commented Oct 22, 2020

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

@AndersonTorres AndersonTorres commented Oct 22, 2020

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

Copy link
Contributor

@IvarWithoutBones IvarWithoutBones left a comment

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

@AndersonTorres AndersonTorres commented Oct 23, 2020

Also, squash the commits.

djanatyn and others added 2 commits Oct 27, 2020
Co-authored-by: r-burns <52847440+r-burns@users.noreply.github.com>
@djanatyn
Copy link
Contributor Author

@djanatyn djanatyn commented Oct 27, 2020

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

@AndersonTorres AndersonTorres commented Oct 28, 2020

LGTM, LGTBorg.

@AndersonTorres AndersonTorres merged commit c13da09 into NixOS:master Oct 28, 2020
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants