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

go: do not remove regexp/syntax/make_perl_groups.pl #193493

Closed
wants to merge 1 commit into from

Conversation

tie
Copy link
Member

@tie tie commented Sep 29, 2022

Description of changes

This change invokes bash interpreter directly on make.bash script. This allows using overrideAttrs with dontPatch set to true as a workaround for issue #125198.

pkgs.go.overrideAttrs (prev: { dontPatch = true; })

The package also now keeps the regexp/syntax/make_perl_groups.pl file since it is not used by the test suite and it is confusing when some files present in upstream src tree are missing.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@zowoq
Copy link
Contributor

zowoq commented Sep 29, 2022

Can we move patchShebangs instead as I suggested in #193458 (comment)?

@tie tie changed the title Go make bash go: invoke bash directly on make.bash Sep 29, 2022
@tie
Copy link
Member Author

tie commented Sep 29, 2022

Can we move patchShebangs instead as I suggested in #193458 (comment)?

Assuming dontPatch = true, wouldn’t that potentially cause unexpected side effects (e.g. patching some codegen scripts in GOROOT), especially since Go builds just fine without patches?

@tie tie force-pushed the go-make-bash branch 2 times, most recently from 1741488 to da96e85 Compare September 29, 2022 10:10
@zowoq
Copy link
Contributor

zowoq commented Sep 29, 2022

I'd prefer moving patchShebangs unless it is demonstrated to be unsuitable.

@zowoq
Copy link
Contributor

zowoq commented Sep 29, 2022

Could just need to check if (go.overrideAttrs (prev: { patchPhase = false; })) works, then we don't need to make any changes (apart from the perl file).

@zowoq
Copy link
Contributor

zowoq commented Sep 29, 2022

Having thought about this some more I don't think bash make.bash should be merged anyway, it is a workaround for a problem that has only been created by using dontPatch = true;.

@tie
Copy link
Member Author

tie commented Sep 29, 2022

Why buildPhase (and patchPhase for that matter) should patch shebangs even if it not necessary for the build instructions? Isn’t that part of the default fixupPhase and controlled with dontPatchShebangs?

This change keeps the regexp/syntax/make_perl_groups.pl file even though
it contains invalid shebang. It is not used by the test suite and there
is no reason to remove it. In fact, it is confusing when some files used
to generate Go code in standard library are missing in the package.
@tie
Copy link
Member Author

tie commented Sep 29, 2022

Honestly, I don’t see bash make.bash as a workaround, that’s how I’ve been building Go releases for years. That said, I’m fine with dropping that part of the PR, Nixpkgs is flexible enough so I can keep doing things how I’m used to on my machines without forcing my opinions on others.

@zowoq zowoq changed the title go: invoke bash directly on make.bash go: do not remove regexp/syntax/make_perl_groups.pl Sep 29, 2022
@@ -138,9 +138,6 @@ stdenv.mkDerivation rec {

preInstall = ''
rm -r pkg/obj
# Contains the wrong perl shebang when cross compiling,
# since it is not used for anything we can deleted as well.
rm src/regexp/syntax/make_perl_groups.pl
Copy link
Contributor

Choose a reason for hiding this comment

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

@Mic92 You added this in 79b9462, are you fine with dropping it?

@tie
Copy link
Member Author

tie commented Mar 17, 2023

Closing this as there was no feedback for some time and I’m no longer interested in this change — I’ve been using a custom unpatched Go derivation for builds (Go modules that are sometimes built by other developers that don’t use Nix), and it doesn’t really matter whether Go compiler used for programs in NixOS is patched or not.

@tie tie closed this Mar 17, 2023
@tie tie deleted the go-make-bash branch July 8, 2023 04:52
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.

2 participants