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: invoke bash directly on make.bash #193458

Closed
wants to merge 122 commits into from
Closed

go: invoke bash directly on make.bash #193458

wants to merge 122 commits 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 script since it is not used by the current 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.

trobert and others added 30 commits May 25, 2021 13:52
Pass `-t` to pixz to prevent it from appending an index to the end of
the uncompressed stream, confusing tools such as `machinectl import-tar`.

Fixes: NixOS#187816
Copy link
Contributor

@zowoq zowoq left a comment

Choose a reason for hiding this comment

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

  • Needs to go to staging, not master.

  • Split the bash and perl changes into separate commits.

  • Add a comment in each file above the bash change:

# invoke the bash interpreter directly on make.bash script.
# this allows using overrideAttrs with dontPatch set to true.

@zowoq
Copy link
Contributor

zowoq commented Sep 29, 2022

This change invokes bash interpreter directly on make.bash script.

Rather than doing this it might be easier if we move patchShebangs to buildPhase?

diff --git a/pkgs/development/compilers/go/1.18.nix b/pkgs/development/compilers/go/1.18.nix
index 7b208da5988..5390d4f2b5b 100644
--- a/pkgs/development/compilers/go/1.18.nix
+++ b/pkgs/development/compilers/go/1.18.nix
@@ -63,10 +63,6 @@ stdenv.mkDerivation rec {
 
   depsTargetTarget = lib.optional stdenv.targetPlatform.isWindows threadsCross;
 
-  postPatch = ''
-    patchShebangs .
-  '';
-
   patches = [
     (substituteAll {
       src = ./iana-etc-1.17.patch;
@@ -117,6 +113,7 @@ stdenv.mkDerivation rec {
 
   buildPhase = ''
     runHook preBuild
+    patchShebangs .
     export GOCACHE=$TMPDIR/go-cache
     # this is compiled into the binary
     export GOROOT_FINAL=$out/share/go

This change invokes bash interpreter directly on make.bash script.
This allows using overrideAttrs with dontPatch set to true (but also
doCheck set to false for Go 1.17) as a workaround for issue NixOS#125198.

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

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

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

	pkgs.go_1_17.overrideAttrs (prev: {
	  dontPatch = true;
	  doCheck = false;
	})
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.
@zowoq
Copy link
Contributor

zowoq commented Sep 29, 2022

Please open a new PR on staging.

@zowoq zowoq closed this Sep 29, 2022
@tie
Copy link
Member Author

tie commented Sep 29, 2022

Oops, sorry, forgot to rebase!

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.