-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
appimageTools.wrapAppImage: remove version from pname
#271071
Conversation
Result of 1 package blacklisted:
86 packages failed to build:
72 packages built:
|
pname
pname
c243b91
to
8ebb958
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please split this out into two PRs. One making it possible to pass pname + version into appimage tools and one changing all the bad name
usages in packaged appimages to land after the first one. It's hard to review the actual change when there's 82 commits that don't really have anything to do with it. Honestly, the adjustments should probably all be one big tree-wide:
commit.
I've tried adding pname support to appimage tools "properly" a little while ago but haven't found the time to finish it yet.
name = if args ? name && args.name != null then args.name else "${pname}-${version}"; | ||
pname = if args ? pname && args.pname != null then args.pname else lib.getName args.name; | ||
version = if args ? version && args.version != null then args.version else lib.getVersion args.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in #262506, I don't think we should ever "infer" pname from a name when we could instead pass pname directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda agree, but would the alternative be to make pname required, or to fallback to name? That issue seems intent on not affecting existing packages while i figured there aren't that many to fix yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump. Personally i'd like to decouple the basename of the resulting $out/bin
binary produced in buildFHSEnvBubblewrap
from pname
and name
entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ftr, all the packages i fix in this PR now passes in pname
explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good idea and I'm for an additional binaryName
arg that defaults to pname
but it has nothing to do with the concern I voiced here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first building a combined name and then trying to parse the individual parts out again
This is not the case in this PR. If pname or version is passed in then they are used as is with no modification. Only if pname or version is missing are they extracted from name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a better fix for this issue would be to adjust wrapType2 to take version + pname without name and stop passing custom combined names everywhere.
EDIT: (emphasis mine)
I can move name
into args
as well, but cases like this still call for a combined name
. I'll see what i can do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got my changes cleaned up. There's still some bugs left to squish but just so that we don't duplicate work here: Atemu@258615c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And duplicate we did. I went in a direction where we don't use null
, which greatly simplifies the logic
building electron a couple of times took a while. Result of 1 package blacklisted:
4 packages failed to build:
154 packages built:
The remaining failures are due to |
Is it preferred to merge a mass breakage?
The commits view is great, and you only need to check the top two. I could squash it if you prefer. |
No, it should stay compatible anyways.
I'd prefer one commit because it's much easier to digest and also revert. It's also easier to tell it's a tree-wide change when you're looking at the history of any file touched by this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cleanup you wished for.
IMO the final commit (02266f6299781f0e86fdee2cb4de749428f51dea) is somewhat pedantic and should be removed.
@@ -233,7 +232,7 @@ in runCommandLocal name { | |||
''; | |||
inherit args fhsenv; | |||
}; | |||
} '' | |||
}) '' | |||
mkdir -p $out/bin | |||
ln -s ${bin} $out/bin/${pname} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately, this line is why we cannot get rid of pname = args.pname or (lib.getName args.name)
. The fix is to decouple the binary name from the package name but that is way out of scope for this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. We can get rid of it right now. If you're explicitly passing a name
with a version in it, you will get an executable with exactly that name; that's how it should work.
If you've got pname and version, you should simply pass those and you will get "correct" behaviour.
We should not add code for potential wrong usages that violates the principle of least surprise. It's conceivable for example that an intended package name may look like a version or that a user might want the executable to be versioned. Those use-cases should not be forbidden because someone might perhaps use it wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. With #271071 (comment) answered the fix becomes quite simple, please view 49ea55661e278c5fa6d05e86ba2cdb9a1ef0a94f
Result of 1 package blacklisted:
4 packages failed to build:
154 packages built:
|
Could you spin out the ~80 commits that touch packages into another PR? Ideally into one For now they're extremely hard to review and block the merge due to conflicts. |
49ea556
to
7e3db64
Compare
Result of 1 package blacklisted:
57 packages failed to build:
101 packages built:
|
ca7c8d3
to
f283b63
Compare
Is there any interest in merging this? If so please either run a nixpkgs-review right before doing so or let me know, because more affected packages keep being added and i'd like to have a list of what to fix in #273719 |
This looks like a great improvement and would like to see it merged. From my quick look at the changes they look good, but I explicitly didn't think about potential breakages because I'm assuming they'll be fixed in #273719 I found this PR because I'm documenting some stuff related to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple smaller things.
I'd also like to see another nixpkgs-review for basic QA. Currently not near my x86_64-linux machine, so I can't easily do this myself.
If that comes back positive, this is ready I think :)
f283b63
to
ce7e47d
Compare
I seem to have forgot to add Result of 3 packages marked as broken and skipped:
1 package blacklisted:
63 packages failed to build:
99 packages built:
EDIT: i'm a silly goose. Creating a full rebuild is easy, just touch some white space. Merge this whenever |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All commits that touch the same attrpath have already been squashed.
You didn't have to go quite that far; my concern was only that a significant portion of code was added and then removed again.
There's likely one or two packages that were missed but that's okay when we're touching hundreds of packages.
One thing I just remembered is that this is a breaking change. You should probably add a release note for that.
The doc/build-helpers/special/fhs-environments.section.md
should also be updated to reflect the new arguments that should be used.
I'd then also like to see a final nixpkgs-review. You still have the sources local, right?
Diff LGTM.
`appimageTools.wrapType2` no longer creates a binary `$out/bin/${name}` if `pname` and `version` is provided. Derivations that have worked around this behavior with a `mv $out/bin/{${name},${pname}}` broke as a result. This should fix most instances. contex: NixOS#271071
bddb859
to
17151c6
Compare
nixpkgs-review in progress |
Result of 2 packages marked as broken and skipped:
2 packages blacklisted:
5 packages failed to build:
168 packages built:
failures are due to |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/48 |
Pass attributes `pname` and `version` directly into appimage.wrapType2, instead of attribute set update ('//'), to ensure that they survive across overriding. This is possible as the pname-version passing of buildFHSEnvBubblewrap, buildFHSEnv, and appimageTools.wrapAppImage is fixed.[1] [1]: NixOS#271071
NixOS/nixpkgs#271071 Didn't realize this was a thing which would explain the previous breakages.
this works since NixOS#271071
Description of changes
Prior to this PR:
With this PR:
The erroneous
pname
s has been the source of many issues, especially since the wrapped software tends to be unfree.nixpkgs.config.allowUnfreePredicate
, even when usinglib.getName
would need to include the version number. Fixes like this will become a thing of the past.An other common fix we don't need to apply any more is
mv $out/bin/$pname $out/bin/${pname}
to strip the version from the resulting binary inbin/
. Here is a listing of the likely affected files which this PR will fix before dropping theWIP:
prefix:rg wrapType -l | xargs grep mv
I'll run a nixpkgs-review over night
closes #262506 (parallel effort)
closes #262350
fixes#267508
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Priorities
Add a 👍 reaction to pull requests you find important.