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

bambootracker: init at 0.4.3 #81840

Merged
merged 1 commit into from Jul 31, 2020
Merged

Conversation

OPNA2608
Copy link
Contributor

@OPNA2608 OPNA2608 commented Mar 5, 2020

Motivation for this change

Been in my local checkout for awhile, decided to push since it was mentioned in #81815.

Ping @fgaz because you were interested in packaging it. Give it a look, let me know how it looks to you.

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) (new package -> 326780408)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@OPNA2608
Copy link
Contributor Author

OPNA2608 commented Mar 6, 2020

Seems to eval fine on x86 & ARM Linux, maybe a trusted user could kick off a Darwin build for completeness' sake? 🙂

@OPNA2608 OPNA2608 force-pushed the package-bambootracker branch 2 times, most recently from ed41722 to 6654ac3 Compare March 15, 2020 17:38
@OPNA2608
Copy link
Contributor Author

Updated to new v0.4.0 release.

@OPNA2608 OPNA2608 changed the title bambootracker: init at 0.3.5 bambootracker: init at 0.4.0 Mar 27, 2020
@OPNA2608
Copy link
Contributor Author

OPNA2608 commented Apr 8, 2020

Noticed that I missed the i18n & skin files, added those. Does copying all the licensing files to share/BambooTracker make sense or is that abit too excessive?

@fgaz
Copy link
Member

fgaz commented Apr 13, 2020

I'd like to know that too. In my packages I didn't bother to include them if the makefile/whatever didn't do it by itself, but maybe I should have.

@OPNA2608 OPNA2608 force-pushed the package-bambootracker branch 2 times, most recently from 6a20f2e to 8c32baa Compare April 21, 2020 20:40
@OPNA2608
Copy link
Contributor Author

Updated to new v0.4.1 release.

We added a workaround for the missing installation of i18n files on our side (it's caused by nixpkgs' qtbase version, QTBUG-77398), so I removed that part of the postInstall phase.

@OPNA2608 OPNA2608 changed the title bambootracker: init at 0.4.0 bambootracker: init at 0.4.1 Apr 22, 2020
@OPNA2608
Copy link
Contributor Author

Bumped again to v0.4.2.

@OPNA2608 OPNA2608 changed the title bambootracker: init at 0.4.1 bambootracker: init at 0.4.2 May 14, 2020
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/175

@OPNA2608 OPNA2608 changed the title bambootracker: init at 0.4.2 bambootracker: init at 0.4.3 Jun 28, 2020
@OPNA2608
Copy link
Contributor Author

Bumped to v0.4.3.

@fgaz
Copy link
Member

fgaz commented Jun 29, 2020

@GrahamcOfBorg build bambootracker

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/195

@fgaz
Copy link
Member

fgaz commented Jul 31, 2020

Let's see if this attracts some committer :)

@OPNA2608
Copy link
Contributor Author

Thanks for reminding me that this PR is still open.

I added a patch to fix the Darwin build, it can be dropped with the next release. Additionally, I ran nixpkgs-fmt on the whole file.

Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

Jikes, I am sorry that merging this took so long! I added some small comments so that the derivation follows the guidelines. It would also be nice if you could squash the two commits into one.

Feel free to ping me when you have incorporated these changes, so that we can get this merged!

pkgs/applications/audio/bambootracker/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/bambootracker/default.nix Outdated Show resolved Hide resolved
Comment on lines 29 to 31
postPatch = "cd BambooTracker";

# sourceRoot = "source/BambooTracker";
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason sourceRoot is not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes patch from GitHub commit fail.

patching sources
applying patch /nix/store/05w1rpcw9j4y1wik5bqblhc4vnzhmdkv-45346ed99559d44c2e32a5c6138a0835b212e875.patch
can't find file to patch at input line 3

@OPNA2608
Copy link
Contributor Author

@danieldk

  • Rebased so the new license value can be used (this predated that change by months)
  • Squashed commits
  • Fixed the extra dot

Can't use sourceRoot due to the patch.

@OPNA2608
Copy link
Contributor Author

Am I doing something wrong btw? GitHub CI is flooding me with "Run failed" emails.

@danieldk
Copy link
Contributor

Thanks for making the changes! Not sure where the GitHub CI errors are coming from. But ofborg is happy.

@danieldk danieldk merged commit b1bbc58 into NixOS:master Jul 31, 2020
@OPNA2608
Copy link
Contributor Author

Thanks for merging! 👍

@OPNA2608 OPNA2608 deleted the package-bambootracker branch September 27, 2022 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants