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

Meson's tool env var interpretation clashes with Autoconf's, leading to disto packaging issue #3969

Closed
Ericson2314 opened this issue Aug 2, 2018 · 18 comments

Comments

@Ericson2314
Copy link
Member

Ericson2314 commented Aug 2, 2018

With Autoconf (along with ideomatic macros):

  • <TOOL>_FOR_BUILD affects the build platform
  • <TOOL> affects the host platform
  • <TOOL>_FOR_TARGET affects the target platform.

For example, if I am building a Canadian cross GCC:

  • CC_FOR_BUILD builds tools on the fly used to generate some repetitive C code.
  • CC builds the cross compiler itself
  • CC_FOR_TARGET builds libgcc and other runtime libraries, since the newly-built compiler cannot be run to build them.

Meson however assumes CC, AS and friends always target the native platform, whatever that is. Notice how in terms of "build", "host" and "target", rather than "native", this means the meaning of these environment variables in Meson is conditional on whether we are cross compiling or not. CC is explicitly used in the native case for building things morally for the host platform (i.e. stuff that will be installed), but not in the cross case.

In the distro I package use, NixOS, We define the variables according to Autoconf https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/bintools-wrapper/setup-hook.sh#L66 here. (Granted there's a number of abstractions going on so I don't expect that to be clear.) This works in the fast majority of cases, but creates issues for cross-compiling our packages that use Meson, even though we also create a cross file just for meson. It sees our CC, targeting the host platform, and tries to use that rather than CC_FOR_BUILD for building code gen tools. This fails as the resulting binaries cannot be run at build time.

At a minimum, this issue would be solved if Meson would ignore CC for building code gen tools in the cross case. It would be nicer still if Meson would read <TOOL>_FOR_BUILD rather than <TOOL> in that cross case. Even better would be using <TOOL>_FOR_BUILD for building code gen stuff in all cases: this would make the native in cross cases more consistent. (It would also help with e.g. optimizing release builds but not codegen tools used for release buidls, FWIW.)

Since environment variables are not an idiomatic way to give Meson information, it would also be nice to specify the tools with (something like) the cross file. I opened #3282 for this purpose, but it got no response. Note that this is very analogous to specifying build_machine information more broadly, I proposed in #3689 but seems to be approaching rejected.

CC @dezgeg @jtojnar


So since I opened this issue, we've had two big changes internally:

  1. native file support #4216 native file support lands
  2. Parsing of cross file upfront, and store in cross-agnostic data structures #4445 all binary input (environment variables and config files) goes through BinaryTable class.

This makes solving this a lot easier. I suggest:

  1. Native files have a [binaries] section, which functions in the obvious way (like the env vars today) done, my mistake.
  2. FOO_FOR_BUILD always overrides FOO for the binaries for build (environment.binaries.build).
  3. FOO_FOR_BUILD is the only way to specify for-build binaries for cross builds. Plain FOO only controls host binaries for cross builds.

In particular, 1 and 2 and land in isolation, and are not breaking changes. Only 3 is, and by then the idiomatic way to control binaries would be the native file anyways.

CFLAGS and friends are more complicated, as the code paths are still wildly different.

@nirbheek
Copy link
Member

nirbheek commented Aug 7, 2018

Yes, we do have an inconsistency here with Autoconf. CC and friends always mean build tools on Meson and CC and friends always mean host tools on Autoconf.

It is too late to change this, IMO.

@nirbheek
Copy link
Member

nirbheek commented Aug 7, 2018

Note that this is very analogous to specifying build_machine information more broadly, I proposed in #3689 but seems to be approaching rejected.

The mechanism is rejected, but the concept is not. We really want it because environment variables are not the greatest way to specify configuration, particularly paths. For instance, there is no way to reliably disambiguate spaces in paths from spaces separating arguments. You could in theory use shell quoting, but in practice most tools don't implement it (meson does, though) so it's difficult to use it consistently.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Aug 7, 2018

CC and friends always mean build tools on Meson

No this isn't true. It's very important to understand that the build, host, and target machines are defined for native and cross builds alike:

  • build machine: where the code is built
  • host machine: where the (installed) code is run
  • target machine: where code that's emitted will run.

Note that nothing here says the machines are different. For native builds they all coincide, for cross builds they don't. CC is definitely used to make installed code in the native case, so still is the C compiler for host, in addition to build. If it was only the C compiler for build, one could only make code gen tools (and other intermediate things), not compile anything that will be installed. Clearly, that is not the case.

I know I sound like a pedant in the previous paragraph, but this shift in mindset is key to removing all the is_cross as I mention in #3972, and making this change actually backwards compatible. Consider these cases:

  1. CC no --cross-file: CC_FOR_BUILD=$CC default kicks in. Same as Autoconf and matches Meson today.
  2. No env vars, no --cross-file: CC is inferred, and CC_FOR_BUILD=$CC kicks in after. Same as Autoconf and matches Meson today.
  3. CC, CC_FOR_BUILD, no --cross-file: no defaulting is needed. Same as Autoconf, only a breaking change for Meson if we say today uses could rely on CC_FOR_BUILD being ignored.
  4. No env vars, --cross-file: CC comes from cross file, CC_FOR_BUILD is inferred: Morally matches Autoconf, compatible with today
  5. CC_FOR_BUILD, --cross-file: CC comes from cross file: Morally matches Autoconf, only a breaking change for Meson if we say today uses could rely on CC_FOR_BUILD being ignored.
  6. CC, CC_FOR_BUILD, --cross-file: CC comes from cross file, not env var, CC_FOR_BUILD explicitly set. CC env var is thus overridden and ignored: Morally matches Autoconf, only a breaking change for Meson if we say today uses could rely on CC_FOR_BUILD being ignored.
  7. CC, --cross-file: CC comes from cross file: CC_FOR_BUILD=$CC: Only case which doesn't match Autoconf: We use CC to define CC_FOR_BUILD rather than ignoring it and inferring CC_FOR_BUILD for backwards compat. This would be deprecated and eventually removed.

In short, if we allow CC_FOR_BUILD to sometimes overriding CC as an allowed change, there's no breakage: if CC_FOR_BUILD isn't specified everything works as before. All these cases are condensed into the simple rule:

  1. build C compiler = CC_FOR_BUILD, CC, or inferred
  2. host C compiler = from --cross-file,CC, or inferred

If the last rule is eventually dropped after transition period, the rule becomes:

  1. build C compiler = CC_FOR_BUILD, CC when build=host, or inferred
  2. host C compiler = from --cross-file, CC, or inferred

Which is Autoconf's, plus the --cross-file case.

C.f. Meson's rule today is:

  1. build C compiler = CC, or inferred
  2. host C compiler = from --cross-file, CC, or inferred

@jpakkane
Copy link
Member

jpakkane commented Aug 7, 2018

Environment variables are hiddne global state. And thus terrible. We try to minimize their use as much as possible in favor of command line arguments and conf files. It is extremely unlikely we will add more environment variables in in Meson and in fact would like to reduce them further if possible.

@Ericson2314
Copy link
Member Author

@jpakkane it would also be fine with me if Meson stopped interpreting CC. Either ignoring CC_FOR_BUILD and CC completely or interpreting them according to Autoconf would solve my immediate issue of the clashing interpretations.

I thought https://mesonbuild.com/Quick-guide.html#using-meson-as-a-distro-packager meant we had to keep around some env vars for distro's sake, and if we must keep CC than I still highly recommend CC_FOR_BUILD. But I have no problem getting rid of either and, as a distro user, could work around it.

@jpakkane
Copy link
Member

jpakkane commented Aug 8, 2018

I would love to drop CC but we can't because that is the default for every distro and other such toolchains on the planet. 😢

Also we'd first need to get some other way of defining the compiler, such as the build machine file discussed elsewhere.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Aug 8, 2018

Alright. Then let's just let this one sit until #3972 is figured out.

@dezgeg
Copy link

dezgeg commented Aug 8, 2018

I understand the desire to avoid environment variables; in NixOS they have caused a bunch of problems when different build systems interpret the same variables in a different way. For instance, a bunch of packages give incomprehensible link errors when LD is set to the ld binary directly, because they expect LD to be gcc which knows how to link the C startup libraries etc. which plain ld doesn't. And other bunch of packages expect STRIP to hold the flags to give to strip, not the command used for stripping which again leads to exciting error messages.

How about having the proposed --build-file or --native-file or whatever take precedence over the CC etc. environment variables? I think that would solve the problem described in this ticket in a backwards-compatible way.

@Ericson2314
Copy link
Member Author

@dezgeg that's what #3972 is for.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Aug 8, 2018

@jpakkane For the record in #3782 someone else set CC running into the same issue; Let's do figure out the explicit file way first, but for when we come back to this I do think CC/CC_FOR_BUILD should be defined according to autoconf when the purpose is distro/legacy compat; see #3782 (comment) where that same person was miffed that defining CC as usual caused problems.

@dezgeg
Copy link

dezgeg commented Aug 8, 2018

I don't see #3972 having a single mention that the file would override CC.

@Ericson2314
Copy link
Member Author

@dezgeg Amended the OP to clarify that.

@ePirat
Copy link
Contributor

ePirat commented Jan 8, 2019

We hit a problem with this in VLCs contrib system too and had to special-case things for meson, so personally while I think meson should not start interpreting the env variables in the way autotools does, for cross, I think it should completely ignore them as else it just clashes with how every other build system I know of handles this, which is an annoying problem.

While it might be easy to suggest "just unset all those env variables", this is in some cases not easily doable.

But yes this is quite a big change with regard to backwards compatibility so not sure if it is not too late for that now.

@Ericson2314
Copy link
Member Author

@ePirat I edited the OP to mention what's landed since I opened the issue, and what concrete steps I recommend now. I take it you at least agree with step 1, without which there would be no way to specify those binaries if cross builds ignore the env vars?

@ePirat
Copy link
Contributor

ePirat commented Jan 8, 2019

Yes, definitely.

@Ericson2314
Copy link
Member Author

Oops, brain fart. [binaries] is the one section that machine files already have. We need [properties] in there so that CFLAGS etc isn't needed in the cross case.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jan 8, 2019

But there is the analogous change addition of [properties] to the machine file so that CFLAGS is never needed that I assume you also would want for the exact same reason.

Ericson2314 added a commit to Ericson2314/meson that referenced this issue Dec 18, 2019
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Dec 18, 2019
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Dec 18, 2019
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Dec 18, 2019
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Dec 18, 2019
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Dec 18, 2019
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Dec 18, 2019
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Dec 18, 2019
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Dec 19, 2019
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Jan 8, 2020
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Jan 8, 2020
@Ericson2314
Copy link
Member Author

#6363 is the PR for this, FYI

Ericson2314 added a commit to Ericson2314/meson that referenced this issue Mar 6, 2020
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Mar 6, 2020
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Mar 6, 2020
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Mar 12, 2020
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Mar 19, 2020
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Mar 19, 2020
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Mar 19, 2020
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Mar 20, 2020
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Mar 21, 2020
Ericson2314 added a commit to Ericson2314/meson that referenced this issue Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants