Skip to content

buildEnv: builder.pl: use signature and move the ignoreSingleFileOutputs parameter after the collision-related ones#364203

Merged
philiptaron merged 2 commits intoNixOS:stagingfrom
ShamrockLee:buildenv-signature
Dec 12, 2024
Merged

buildEnv: builder.pl: use signature and move the ignoreSingleFileOutputs parameter after the collision-related ones#364203
philiptaron merged 2 commits intoNixOS:stagingfrom
ShamrockLee:buildenv-signature

Conversation

@ShamrockLee
Copy link
Contributor

This PR uses Perl's signatures feature to increase readability and maintainability. This makes errors related to functional argument passing more discoverable.

Perl's signatures feature was added in Perl 5.20 and stabilized in Perl 5.32. As the Perl version in Nixpkgs release-24.05 (the previous stable release branch) is already 5.38.2, it should be safe to use such a feature in current Nixpkgs.

This PR also includes a commit to move the ignoreSingleFileOutputs argument after the ones that deal with collisions (to be exact, after priority). I added right after ignoreCollisions in #353752 for the convenience of my development but realized later that it would look awkward when reading through the function call.

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@nix-owners nix-owners bot requested a review from philiptaron December 11, 2024 11:48
@github-actions github-actions bot added 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Dec 11, 2024
@ShamrockLee
Copy link
Contributor Author

I'll add some package tests for buildEnv to make the builder.pl changes safer. Before that, I'm marking this PR as a draft.

@ShamrockLee ShamrockLee marked this pull request as draft December 11, 2024 16:44
@philiptaron philiptaron requested a review from stigtsp December 11, 2024 17:16
@philiptaron
Copy link
Contributor

philiptaron commented Dec 11, 2024

I added @stigtsp (who is busy and under no compulsion to review) as an FYI for their Perl expertise.

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

Adding tests is a good idea. The Perl passes my reading of it and a quick build on Big Computer. The parameter order changes to keep collision-related parameters together make sense.

Copy link
Member

@stigtsp stigtsp left a comment

Choose a reason for hiding this comment

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

Code LGTM

Using signatures is a great idea. This feature has in practice been stable for a long time and is commonly used.

@@ -1,6 +1,7 @@
#! @perl@ -w

use strict;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
use strict;
use v5.40;

Using v5.40 also enables strict, and requires that version (or later) of perl, and enables other features like try/catch, say etc.

Not strictly needed for this PR of course - so feel free to not include this suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since Perl is at 5.40.0 on the stable branch, it should be safe to add such a requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's do it in a new PR instead.

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

Actually, while a test would be great, I would also be fine merging as-is.

@ShamrockLee ShamrockLee marked this pull request as ready for review December 12, 2024 05:54
@ShamrockLee
Copy link
Contributor Author

@stigtsp Just tested vim.customize { vimrcConfig = { }; } and got some Perl errors:

vim> Bareword filehandle "DIR" not allowed under 'no feature "bareword_filehandles"' at /nix/store/dfgbszcdkp1i3ksbh4gpyify183v122p-builder.pl line 78.
vim> Bareword filehandle "DIR" not allowed under 'no feature "bareword_filehandles"' at /nix/store/dfgbszcdkp1i3ksbh4gpyify183v122p-builder.pl line 79.
vim> Bareword filehandle "DIR" not allowed under 'no feature "bareword_filehandles"' at /nix/store/dfgbszcdkp1i3ksbh4gpyify183v122p-builder.pl line 80.
vim> Bareword filehandle "PROP" not allowed under 'no feature "bareword_filehandles"' at /nix/store/dfgbszcdkp1i3ksbh4gpyify183v122p-builder.pl line 205.
vim> Bareword filehandle "PROP" not allowed under 'no feature "bareword_filehandles"' at /nix/store/dfgbszcdkp1i3ksbh4gpyify183v122p-builder.pl line 206.
vim> Bareword filehandle "PROP" not allowed under 'no feature "bareword_filehandles"' at /nix/store/dfgbszcdkp1i3ksbh4gpyify183v122p-builder.pl line 207.
vim> Bareword filehandle "FILE" not allowed under 'no feature "bareword_filehandles"' at /nix/store/dfgbszcdkp1i3ksbh4gpyify183v122p-builder.pl line 219.
vim> Bareword filehandle "FILE" not allowed under 'no feature "bareword_filehandles"' at /nix/store/dfgbszcdkp1i3ksbh4gpyify183v122p-builder.pl line 220.
vim> Bareword filehandle "FILE" not allowed under 'no feature "bareword_filehandles"' at /nix/store/dfgbszcdkp1i3ksbh4gpyify183v122p-builder.pl line 221.
vim> Bareword filehandle "FILE" not allowed under 'no feature "bareword_filehandles"' at /nix/store/dfgbszcdkp1i3ksbh4gpyify183v122p-builder.pl line 255.
vim> /nix/store/dfgbszcdkp1i3ksbh4gpyify183v122p-builder.pl has too many errors.

@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. labels Dec 12, 2024
@ShamrockLee
Copy link
Contributor Author

@stigtsp Since use v5.40 requires extra fixes, let's do it in a separate PR instead. I'll drop the commit from this PR.

@ShamrockLee
Copy link
Contributor Author

The change has been reset, and vim.customize { vimrcConfig = { }; } now builds.

@philiptaron, should we merge now?

@stigtsp
Copy link
Member

stigtsp commented Dec 12, 2024

@stigtsp Since use v5.40 requires extra fixes, let's do it in a separate PR instead. I'll drop the commit from this PR.

If it doesn't work with the feature flags from 5.40, then it's fine to not include that change of course. 👍

@github-actions github-actions bot removed 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. labels Dec 12, 2024
@philiptaron philiptaron merged commit d752ad6 into NixOS:staging Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants