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

gettext: Don't use envHooks anymore #33524

Merged
merged 1 commit into from
Jan 7, 2018
Merged

Conversation

srhb
Copy link
Contributor

@srhb srhb commented Jan 6, 2018

Motivation for this change

After conferring with @jtojnar on irc, we believe this was simply missed in #26805

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@srhb srhb changed the base branch from master to staging January 6, 2018 14:06
@srhb srhb changed the base branch from staging to master January 6, 2018 14:07
@srhb srhb force-pushed the gettext-addEnvHooks branch from 7486fdf to 95ecb1f Compare January 6, 2018 14:11
@srhb srhb changed the base branch from master to staging January 6, 2018 14:11
@srhb
Copy link
Contributor Author

srhb commented Jan 6, 2018

Rebased onto staging.

@joachifm joachifm requested a review from Ericson2314 January 6, 2018 14:52
@jtojnar jtojnar added the 1.severity: mass-rebuild This PR causes a large number of packages to rebuild label Jan 6, 2018
@orivej
Copy link
Contributor

orivej commented Jan 6, 2018

Then other instances of envHooks+= should be replaced as well?

@srhb
Copy link
Contributor Author

srhb commented Jan 6, 2018

Probably. There's three left:

data/misc/tzdata/tzdata-setup-hook.sh
applications/misc/gnuradio/default.nix
development/libraries/dleyna-core/setup-hook.sh

This one is the only one holding back the channel though, as far as I can see.

@Ericson2314
Copy link
Member

Thanks! Just need to make sure we use the right offset.

@srhb
Copy link
Contributor Author

srhb commented Jan 6, 2018

It is a very naïve fix. I don't know if it is the right offset.

@orivej
Copy link
Contributor

orivej commented Jan 6, 2018

@Ericson2314 The documentation says that "These 6 bash variables (envBuildBuildHooks etc.) correspond to the 6 sorts of dependencies by platform", but I don't understand what these sorts mean. For example, what is envBuildTargetHooks?

@Ericson2314
Copy link
Member

So env<Foo><Bar>Hooks gets applied to pkgs<Foo><Bar> only. On native it doesn't matter, for back compat, but let's not count on that!

addEnvHooks <Foo-offset> adds to each env<Foo>*Hooks, cause normally you don't care about the target platform of the dependency being mapped.

Basically, gettext is always a native build input, so use host offset for other native build inputs, or target offset for build inputs.

@orivej
Copy link
Contributor

orivej commented Jan 6, 2018

What is pkgsBuildTarget?

@orivej orivej merged commit 175d890 into NixOS:staging Jan 7, 2018
@Ericson2314
Copy link
Member

That would be things running on the build platform but compiling for the target platform. Imagine you are building a build != host != target (Canadian cross) compiler. You use a build->host compiler to build the compile itself. But you cannot use that to build the new compiler's runtime. You can't use the new compiler either cause you cannot run it. Instead you use a separate build->target compiler instead.

[Ideally we would use the new compiler to build the run time like any other library, on the host platform where thst compiler will actually run, but some compilers insist the runtime be built with the compiler that generates code using it, giving us the need for such a weird dependency.]

srhb added a commit to srhb/nixpkgs that referenced this pull request Jan 7, 2018
This reverts commit d3ad52a.

In order to see if NixOS#33524 is
sufficient
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.severity: mass-rebuild This PR causes a large number of packages to rebuild 10.rebuild-darwin: 501+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants