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: add buildPlatformCanExecuteHostPlatform #220435

Closed
wants to merge 2 commits into from
Closed

stdenv: add buildPlatformCanExecuteHostPlatform #220435

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 10, 2023

cc: @Ericson2314 @alyssais @sternenseemann @trofi for feedback on whether or not this is a good idea.

Description of changes

This PR adds stdenv.buildCanExecuteHost, which is defined as:

  buildCanExecuteHost = stdenv.buildPlatform.canExecute stdenv.hostPlatform;

Motivation

By making the above expression referenceable by the nice short string stdenv.buildCanExecuteHost (which never needs to be parenthesized), I hope people will find it more convenient to type and therefore be more likely to use it instead of stdenv.buildPlatform==stdenv.hostPlatform

I think that stdenv.buildPlatform==stdenv.hostPlatform is almost always the wrong thing, and we should start discouraging it. This PR adds stdenv.buildCanExecuteHost, which is almost always what should be used instead.

Specific Example

My buildfarm is a bunch of AMD Opterons. I recently switched over to expressing the -march= flag for them the correct way: by creating a new local hostPlatform called opteron for them which sets gcc.arch="bdver1" (and equivalent for rustc). Previously I'd just been kludging it into NIX_CFLAGS_COMPILE with an overlay.

I have a few other x86_64 machines which do not support the bdver1 instructions (mostly the AVX/SSE stuff). So these machines will segfault if you try to use an opteron-compiled ffmpeg on them. Therefore, I do two builds of nixpkgs for x86_64 in parallel: one for opteron and one for lib.systems.examples.gnu64 (which is the "vanilla" x86_64).

Here's the problem:

  stdenv.buildPlatform != stdenv.hostPlatform

because

  opteron != lib.systems.examples.gnu64

... but this scenario really is not a cross-compile, except maybe from the perspective the stdenv bootstrap. Most packages that are doing an equality-test between buildPlatform and hostPlatform really just want to know if the build platform can execute host binaries -- usually because upstream's build machinery is broken in that case so features have to be disabled.

Unfortunately we've established stdenv.buildPlatform==stdenv.hostPlatform as the idiom for "is this a cross compile?". So I'd like to make it more convenient for package authors to use stdenv.buildPlatform.canExecute stdenv.hostPlatform, by making it less verbose for them to type.

This PR adds `stdenv.buildCanExecuteHost`, which is defined as:

  buildCanExecuteHost = stdenv.buildPlatform.canExecute stdenv.hostPlatform;

By making the above expression referenceable by the nice short
string `stdenv.buildCanExecuteHost`, I hope people will find it more
convenient to type and therefore be more likely to use it instead of
`stdenv.buildPlatform==stdenv.hostPlatform`

I think that `stdenv.buildPlatform==stdenv.hostPlatform` is almost
always the wrong thing, and we should start discouraging it.  This
PR adds stdenv.buildCanExecuteHost, which is almost always what
should be used instead.

My buildfarm is a bunch of AMD Opterons.  I recently switched over
to expressing the `-march=` flag for them the correct way: by
creating a new local `buildPlatform` called `opteron` for them which
sets `gcc.arch="bdver1"` (and equivalent for `rustc`).

I have a few other `x86_64` machines which do not support the
`bdver1` instructions (mostly the AVX/SSE stuff).  So these machines
will segfault if you try to use an `opteron`-compiled `ffmpeg` on
them.  Therefore, I do two builds of nixpkgs for x86_64 in parallel:
one for `opteron` and one for `lib.systems.examples.gnu64` (which is
the "vanilla" `x86_64`).

Here's the problem:

```
  stdenv.buildPlatform != stdenv.hostPlatform
```

because

```
  opteron != lib.systems.examples.gnu64
```

... but this really a cross-compile from the perspective of
everything outside of `stdenv`.  Most packages that are doing an
equality-test between `buildPlatform` and `hostPlatform` really just
want to know if the build platform can execute host binaries --
usually because upstream's build machinery is broken in that case so
features have to be disabled.

Unfortunately we've established
`stdenv.buildPlatform==stdenv.hostPlatform` as the idiom for "is
this a cross compile?".  So I'd like to make it *more* convenient
for package authors to use `stdenv.buildPlatform.canExecute
stdenv.hostPlatform`, by making it less verbose for them to type.
@Shawn8901
Copy link
Contributor

I like the idea, i am not an expert by any means on the topic, but isn't the architecture inferior list not something that should be used here?

i did not find what canExecute does in detail, either i did overscroll it or am just blind if it's using this.
sadly it's also not filled for your use case.

@trofi
Copy link
Contributor

trofi commented Mar 10, 2023

Fun fact: there is only one instance of canExecute in nixpkgs that checks something other: pkgs/top-level/all-packages.nix: if (!stdenv.buildPlatform.canExecute stdenv.targetPlatform) then. And I am not sure it's correct.

@@ -77,6 +77,8 @@ let

stdenv = (stdenv-overridable argsStdenv);

buildCanExecuteHost = buildPlatform.canExecute hostPlatform;
Copy link
Contributor

Choose a reason for hiding this comment

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

I view this file as almost completely oblivious of internals of buildPlatform or hostPlatform. There is one exception (which we would probably want to remove eventually in favour of explicit stdenv.hostPlatform.isx86_64 & co.):

      inherit (hostPlatform)
        isDarwin isLinux isSunOS isCygwin isBSD isFreeBSD isOpenBSD
        isi686 isx86_32 isx86_64
        is32bit is64bit
        isAarch32 isAarch64 isMips isBigEndian;

There is certain charm of being able to grep for hostPlatform or buildPlatform to see where we peek at the implementation details when building things in individual packages.

The above 2 points very weakly push me to say that I prefer explicit stdenv.buildPlatform.canExecute stdenv.hostPlatform slightly more than external helper.

But then again I almost never use it myself.

Copy link
Author

Choose a reason for hiding this comment

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

There is certain charm of being able to grep for hostPlatform or buildPlatform

This is an excellent point. I have pushed another commit which changes the name to buildPlatformCanExecuteHostPlatform. It's more verbose, but greppability matters.

And yes, I think those inherit (hostPlatform)s should be deleted too.

@ghost
Copy link
Author

ghost commented Mar 10, 2023

but isn't the architecture inferior list not something that should be used here?

Indirectly. Thanks for noticing this!

#220457

... and that has to be resolved (at least over-conservatively) before I can undraftify this PR.

@ghost ghost marked this pull request as ready for review March 12, 2023 02:40
@ghost ghost requested review from Ericson2314 and matthewbauer as code owners March 12, 2023 02:40
@ghost
Copy link
Author

ghost commented Mar 12, 2023

I've undraftified this, but note that #87909 (or its equivalent) needs to merge before we can start recommending that people change hostPlatform!=buildPlatform to stdenv.buildPlatformCanExecuteHostPlatform.

@ghost ghost changed the title stdenv: add buildCanExecuteHost stdenv: add buildPlatformCanExecuteHostplatform Mar 12, 2023
@sternenseemann
Copy link
Member

I feel like this is basically the same as having a stdenv.isCross which we decided against, so at least the argument of consistency applies.

@ghost
Copy link
Author

ghost commented Mar 16, 2023

I feel like this is basically the same as having a stdenv.isCross which we decided against,

Do you have a pointer to the discussion?

My understanding was that stdenv.isCross doesn't exist because "is cross compiling" is vague and not everybody agrees on what it means (or that it means anything useful at all).

Does anybody think buildPlatformCanExecuteHostPlatform means something other than stdenv.buildPlatform.canExecute stdenv.hostPlatform? If so, what?

@sternenseemann
Copy link
Member

Same as with the isCross situation, there are three and not two platforms involved – it is entirely conceivable that stdenv.buildPlatform.canExecute stdenv.targetPlatform is used somewhere. Just seems odd to shorten it when it is not really necessary and we have an established pattern already.

@ghost
Copy link
Author

ghost commented Mar 17, 2023

it is entirely conceivable that stdenv.buildPlatform.canExecute stdenv.targetPlatform is used somewhere.

It is used, in two places. I'm not proposing to change that.

Since Nixpkgs doesn't allow Canadian cross compilers this is a very, very, very unusual and special situation. It only comes up when you have a non-compiler tool with a targetPlatform. Right now that is meson and gobject-introspection.

@sternenseemann it seems like you have aesthetic concerns with this PR, so I'll put it back on the shelf until I deal with #220457

@ghost ghost marked this pull request as draft March 17, 2023 19:01
@ghost ghost mentioned this pull request Apr 6, 2023
15 tasks
@AndersonTorres AndersonTorres changed the title stdenv: add buildPlatformCanExecuteHostplatform stdenv: add buildPlatformCanExecuteHostPlatform Aug 9, 2023
@ghost ghost closed this Oct 22, 2023
@ghost ghost deleted the pr/stdenv/buildCanExecuteHost branch October 22, 2023 07:35
This pull request was closed.
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.

3 participants