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

srcOnly: make the -source suffix optional #333900

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thecaralice
Copy link
Contributor

@thecaralice thecaralice commented Aug 11, 2024

Description of changes

srcOnly has a use as a trivial builder - cases when you just want to apply a patch to some derivation for example. However, it does not give the user full control of its name, since it adds a -source to whatever name the user provides. This PR addresses that by adding a noSuffix argument to srcOnly. The argument is only read when provided directly and not drvAttrs (should that be changed? Let me know).

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@Mic92 Mic92 changed the title feat: allow changing the -source suffix in srcOnly srcOnly: allow changing the -source suffix in srcOnly Aug 11, 2024
@Mic92
Copy link
Member

Mic92 commented Aug 11, 2024

I don't quite get the use case, what advantage does it have to change the suffix to something else? Aesthetics?

@thecaralice
Copy link
Contributor Author

One use case is having a derivation of form ${pname}-${version} without the -source suffix. Maybe it is also the only one? Now that I think of it, it might be better to make the argument a boolean instead.

@thecaralice thecaralice changed the title srcOnly: allow changing the -source suffix in srcOnly srcOnly: make the -source suffix optional Aug 11, 2024
@Mic92
Copy link
Member

Mic92 commented Aug 11, 2024

Okay but what do you do with the source code afterwards. Do you not put pass it on to a different derivation to perform some other build?

@thecaralice
Copy link
Contributor Author

Sometimes I don't. I can't remember any exact details but I think I had a case when I just needed to patch something without copying everything in the installPhase manually.

@philiptaron
Copy link
Contributor

This is an oddly shaped derivation helper prior to this PR.

  • attrset name presence or absence determines semantics
  • attrset names not used are dropped on the floor
  • drvAttrs if specified are overridden by the function
  • installPhase does not run hooks
  • xor behavior with name and pname/version

I'm on mobile reviewing this, so I haven't searched through nixpkgs, but I'm really struggling to see the use case for which this helper was created.

@philiptaron philiptaron requested review from adisbladis and roberth and removed request for philiptaron August 14, 2024 00:06
@thecaralice
Copy link
Contributor Author

Yeah, I found it weird as well, although it does solve my issues. As I see it, it tries to do two different things:

  1. Extract the source of a certain derivation
  2. Trivially build a source by fetching, unpacking and applying patches, but not actually building anything

The first one is the reason of the drvAttrs magic, not running install hooks (the original hooks are not fit for the new installPhase) and dropping most attrs.

Should I do a heavier refactoring?

@philiptaron
Copy link
Contributor

Should I do a heavier refactoring?

I don't know 🤷🏻‍♂️. All I know is that this helper is too weird for me to review changes to, and after reading it, I'll avoid using it in nixpkgs.

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.

3 participants