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

stdenv: make checkInputs native #53440

Merged
merged 1 commit into from Jan 13, 2019
Merged

stdenv: make checkInputs native #53440

merged 1 commit into from Jan 13, 2019

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Jan 5, 2019

We can't run the checkPhase when build != host, so we may as well make
the checkInputs native.

This signicantly improves the situation of Python packages when enabling
strictDeps.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@FRidh FRidh added 1.severity: mass-rebuild 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on 6.topic: stdenv Standard environment labels Jan 5, 2019
@FRidh
Copy link
Member Author

FRidh commented Jan 5, 2019

cc @oxij who added this in 87651b3

We can't run the checkPhase when build != host, so we may as well make
the checkInputs native.

This signicantly improves the situation of Python packages when enabling
strictDeps.
@Ericson2314
Copy link
Member

Mmm libraries/modules/whatever should generally be buildInputs so I don't think you want that stdenv change. I think this points to some other issue. Hopefully I can investigate.

@FRidh
Copy link
Member Author

FRidh commented Jan 5, 2019

In the case of Python it would mean putting the test runner / executables in nativeBuildInputs and test libraries in checkInputs. That would be really silly.

@Ericson2314
Copy link
Member

Ericson2314 commented Jan 6, 2019

Hmm? That doesn't seem wrong on the face of it. I did a little thought experiment earlier today to try to tease out how things should work:

Suppose we are running tests in a cross build with wine. We already run pythonForBuild with the setup stuff. We would then run a windows python binary with the built library/exectuables, it's tests, both of their dependencies, etc.

Wine runs on the build platform (it's host is our build). It doesn't produce machine code but it it accepts it; so we can say it's target platform is windows, our host. So it's build -> host, i.e. in would-be-called-depsBuildHost, i.e. in depsHostTarget.

Python in all cases accepts machine code on the same platform as it runs. We could think either it's host = target or say it doesn't have a target (unconstrained target, build hashes not target sensitive). Under the former interpretation (host = target), pythonForBuild is a depsBuildBuild and regular python is a depsHostHost. Under the later interpretation, we are free to assign the target platform to get less "exotic" dependencies. We can put pythonForBuild in nativeBuildInputs and regular python in buildInputs.

The python library "builds" are machine independent in principle, but of course could link windows-specific stuff, so their host=our host. (And then there's cython pipi or whatever). Most of this is again target agnostic, but if it was (say if somebody wrote an Android-targetting forth compiler in Python haha) we have their target = our target. This means buildInputs (depsBuildHost given fully formulaic names).

So based on the above, checkInputs would indeed be buildInputs. but also python should be a buildInput too, and you said it's a nativeBuildInput. So yes something is off. I suppose making it all nativeBuildInputs would restore that optional host=target for python interpreters I mentioned, and also make sure it's on the PATH with strictDeps. But normal run-time dependencies are buildInputs, and given that to the test "executable" normal dependencies and checkInputs are all immediate or transitive dependencies of the same sort (wrt platforms), I rather instead keep checkInputs as is, but put regular python (for host) in buildinputs, and do PATH=$HOST_PATH:$PATH to make sure the binaries can be run.

@FRidh
Copy link
Member Author

FRidh commented Jan 6, 2019

Suppose we are running tests in a cross build with wine.

Aside from indeed using an emulator during checkPhase, I don't see any way of running the tests in a cross build. In that case, all test dependencies, except the emulator (wine), would indeed be buildInputs.

and do PATH=$HOST_PATH:$PATH to make sure the binaries can be run.

This should be done in the stdenv then for the non-cross case, because this is not just the case with Python. How do we do something like this, for just the checkPhase, without defining it in the checkPhase?

--- Edit

A bit more about the emulator and buildInputs. If we agree we always need an emulator for testing, then in the non-cross case our emulator is simply x: x. Thinking of it like that we can keep checkInputs as it is, and would have to define our emulator (nativeCheckInputs) that executes the checkPhase.

@FRidh
Copy link
Member Author

FRidh commented Jan 6, 2019

In #53445 I'll continue under the assumption we can use checkInputs. We just need to have a solution for making that actually work 😛

@Ericson2314
Copy link
Member

How do we do something like this, for just the checkPhase, without defining it in the checkPhase?

Perhaps we just do that for now, and not worry about the emulator case. Otherwise, perhaps PATH=$HOST_PATH:$PATH and wine are "emulators", except this isn't an immediate useful insight as they are invoked differently.

@matthewbauer
Copy link
Member

If we ever get tests to work on cross builds, we are going to need to resplice everything, so the "correct" behavior is to be determined. Defining checkInputs as "a native build input that is only added when doCheck = true" seems reasonable here. Most likely we will need to resplice all of the nativeBuildInputs so that they can run inside the emulator so we still have access to things like gnumake and coreutils inside the emulator. Some packages may be smart enough to only run necessary parts in the emulator, but most will require that something like ${stdenv.hostPlatform.emulator buildPackages} make check works.

@FRidh
Copy link
Member Author

FRidh commented Jan 13, 2019

As @matthewbauer said, we can improve this later on. Merging...

@FRidh FRidh merged commit a1a5ea5 into NixOS:staging Jan 13, 2019
@FRidh FRidh deleted the checkinputs branch January 13, 2019 13:43
@Ericson2314
Copy link
Member

Err I still think this is a bad idea.

@Ericson2314
Copy link
Member

Most likely we will need to resplice all of the nativeBuildInputs so that they can run inside the emulator so we still have access to things like gnumake and coreutils inside the emulator.

If there really is a wrapper process, one should not be able to access nativeBuildInputs, if build = host, then we get this for free.

@oxij
Copy link
Member

oxij commented Jan 15, 2019 via email

@FRidh
Copy link
Member Author

FRidh commented Jan 15, 2019

This signicantly improves the situation of Python packages when enabling strictDeps.

Why?

Otherwise we would have to split everywhere the test runner (and possibly others) from other test dependencies, and we already have hundreds of expressions already using this. Also, unless we introduce a nativeCheckInputs this will be really confusing.

--- edit

strictDeps separates nativeBuildInputs from buildInputs. You cannot run an executable during a build unless it is in nativeBuildInputs. To support cross-compilation, I am enabling this setting in buildPythonPackage as a first step.

@Ericson2314
Copy link
Member

@oxij the point of strictDeps is to limit the number of things that observe build == host so there are fewer code paths to maintain.

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.

None yet

5 participants