plasticscm-{theme,client-{core,gui}}: init at 11.0.16.9791#416436
plasticscm-{theme,client-{core,gui}}: init at 11.0.16.9791#416436SuperSandro2000 merged 6 commits intoNixOS:masterfrom
Conversation
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/plastic-scm-for-nix-os/40080/3 |
musjj
changed the title
github-actions
bot
added
10.rebuild-darwin: 0
|
Re-running CI due to a odd failure fixed in #416448 (holy cow, what a PR title... :D) |
|
New to NixOS and nixpkgs, how long should I expect it to take for this to be merged? Need this for work, been suffering without it. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5583 |
|
Got impatient and installed from fork. Works perfectly, thank you! |
nixpkgs-ci
bot
added
the
9.needs: reviewer
musjj
changed the title
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5917 |
dtomvan
left a comment
dtomvan
left a comment
There was a problem hiding this comment.
My suggestions may've been a bit vague, so I made a branch with all of my review suggestions realised: https://github.com/dtomvan/nixpkgs/tree/dtomvan/push-kornunpqzvpm
I seem to have broken internationalization there, but it's close enough that you might be able to make something of it with your domain knowledge of this software.
If FHS envs seem like the only way after all, I'd still like to see the restructuring of the packages as I did in my branch, since this is 6 packages where it could've been 3 or so.
| libz, | ||
| plasticscm-client-core-unwrapped, | ||
| }: | ||
| buildFHSEnv { |
There was a problem hiding this comment.
Do you really need an FHS env for these packages or does autoPatchelfHook suffice?
There was a problem hiding this comment.
Yes, buildFSHEnv is required here. I initially started with autoPatchelfHook but it did not work very well. The way some of the dependencies are loaded just necessitates an FSH environment for full compatibility (either that, or some really invasive/unstable patching). See unityhub.
| assert plasticscm-client-gui-unwrapped.version == plasticscm-client-core-unwrapped.version; | ||
| assert plasticscm-client-gui-unwrapped.version == plasticscm-theme.version; |
There was a problem hiding this comment.
I think it's appropriate instead to put all packages into one, plasticscm-client (which probably would merit a toggle for guiSupport or something), where you use let ... in to set the version once as a single source of truth.
There was a problem hiding this comment.
I'm not really sure about this, they're inherently different packages that don't really depend on each other. Some people may only need the GUI application, some may only need the CLI application and others may want both.
nixpkgs-ci
bot
removed
the
9.needs: reviewer
| mv $out/bin/$pname $out/bin/.$pname-fhsenv | ||
|
|
||
| for app in plasticgui semanticmergetool gtkmergetool gluon; do | ||
| makeWrapper $out/bin/.$pname-fhsenv $out/bin/$app \ |
There was a problem hiding this comment.
Please hardcode pname instead.
There was a problem hiding this comment.
Just to clarify $out/bin/$pname was not provided by the original package, but created by buildFHSEnv itself:
Shouldn't it be safe to rely on $pname here, since we essentially have full control of it?
There was a problem hiding this comment.
We should never rely on pname like this. It is an anti pattern we are trying to get rid of.
There was a problem hiding this comment.
I've hardcoded the pname, but I still don't quite understand the reasoning here. Is there a wider discussion on why this is bad for this particular case?
There was a problem hiding this comment.
Interpolating pname is not great as it only matches by accident. We have an issue for overuse of pname
SuperSandro2000
changed the title
|
Please squash all the commits together. We should have one init commit per package in the end. |
musjj
changed the title
|
Proposed a minor eval failure improvement on |
7 participants
A Software Configuration Management system from Unity that tracks changes to source code and any digital asset over time.
The packages are structured like on Debian:
I didn't include the server packages because I don't use it so I don't know how to test it.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.