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

deprecate environment variables like CFLAGS #4664

Closed
dcbaker opened this issue Dec 19, 2018 · 34 comments
Closed

deprecate environment variables like CFLAGS #4664

dcbaker opened this issue Dec 19, 2018 · 34 comments

Comments

@dcbaker
Copy link
Member

dcbaker commented Dec 19, 2018

I want to make a bold proposal, environment variables like CFLAGS should be considered harmful, and be deprecated in favor of the other option we already have -Dc_args.

Let me sum up why environment variables like CFLAGS are bad:

  • Not portable. These are only really used on Unices, they're extremely uncommon on windows
  • Easy to accidentally change, hard to cache. All you have to do is change to a different terminal and without realizing it you've change CFLAGS from something to nothing
  • We only read them at initialization, we warn if they change, but in a project with a lot of configuration like mesa or systemd thats easy to lose. It's also surprising behavior since meson caches -D options and remembers those on reconfiguration
  • We already have another way to do this -D${lang}_args and -D${lang}_link_args
  • autotools doesn't recommend using these variables
  • They're basically inherited from the bad old days of plain makefiles

What I'm proposing:
A deprecation period with a Deprecated: warning printed at the end of the configuration summary, followed by either ignoring them, or having a hard error for them.

Along these lines I'd like to add new options like -Dpkg_config_path and -Dcmake_search_path for adding new paths for pkg-config and cmake to search.

@jpakkane
Copy link
Member

I agree with everything stated. When I originally added the envvar support I felt kind of bad about it, but it was (and quite likey still is) mandatory because distro packaging and a bunch of other things depend on it.

@nirbheek
Copy link
Member

Another reason: when cross-compiling with Autotools CFLAGS are meant for the host env, but we don't support that. CFLAGS are only for the build machine in meson, since cross compilation information is entirely contained in the cross file.

@dcbaker
Copy link
Member Author

dcbaker commented Dec 20, 2018

If that is critical for distros we have a couple of options:

  1. tell them to add -Dc_args=$CFLAGS -Dc_link_args=$LDFLAGS to their meson wrappers. I know for sure that gentoo, arch, and fedora have meson wrappers in their build infrastructure and don't call meson directly
  2. Add a --read-env-vars flag, note that there are many caveats to using it and that it is only intended for systems like distro packaging or CI systems where rebuilding and reconfiguring is not used

I know we have at least a couple of distro guys around, should we ping them and ask?

@jpakkane
Copy link
Member

Let's start with @ignatenkobrain.

@keszybz
Copy link
Contributor

keszybz commented Dec 21, 2018

+100 to this proposal.

I'm not @ignatenkobrain, but the answer for Fedora is pretty easy: can do.

%meson is defined as

    %set_build_flags 
    %{shrink:%{__meson} 
        --buildtype=plain 
        --prefix=%{_prefix} 
        --libdir=%{_libdir} 
        --libexecdir=%{_libexecdir} 
        --bindir=%{_bindir} 
        --sbindir=%{_sbindir} 
        --includedir=%{_includedir} 
        --datadir=%{_datadir} 
        --mandir=%{_mandir} 
        --infodir=%{_infodir} 
        --localedir=%{_datadir}/locale 
        --sysconfdir=%{_sysconfdir} 
        --localstatedir=%{_localstatedir} 
        --sharedstatedir=%{_sharedstatedir} 
        --wrap-mode=%{__meson_wrap_mode} 
        --auto-features=%{__meson_auto_features} 
        %{_vpath_srcdir} %{_vpath_builddir} 
        %{nil}}

and %set_build_flags is defined as

  CFLAGS="${CFLAGS:-%{build_cflags}}" ; export CFLAGS ; 
  CXXFLAGS="${CXXFLAGS:-%{build_cxxflags}}" ; export CXXFLAGS ; 
  FFLAGS="${FFLAGS:-%{build_fflags}}" ; export FFLAGS ; 
  FCFLAGS="${FCFLAGS:-%{build_fflags}}" ; export FCFLAGS ; 
  LDFLAGS="${LDFLAGS:-%{build_ldflags}}" ; export LDFLAGS

So something like this should work as the replacement definition

    %{shrink:%{__meson} 
        --buildtype=plain 
        ...
        --auto-features=%{__meson_auto_features} 
        -Dc_args="${CFLAGS:-%{build_cflags}}"
        -Dc_link_args="${LDFLAGS:-%{build_ldflags}}"
        -Dcxx_args="${CXXFLAGS:-%{build_cxxflags}}"
        -Dcxx_link_args="${LDFLAGS:-%{build_ldflags}}"
        -Dfortan_args="${FCFLAGS:-%{build_fcflags}}"
        -Dfortran_link_args="${LDFLAGS:-%{build_ldflags}}"
        %{_vpath_srcdir} %{_vpath_builddir} 
        %{nil}}

(FC is for F90, and F is for F77, not sure how this maps to meson language names).

@dcbaker
Copy link
Member Author

dcbaker commented Dec 21, 2018

I don't think we currently distinguish between f90 and f77; also it should be -Dcpp_* :)

I'm not an arch maintainer, just a user, but we have this, which should be pretty easy to extend to use -Dc_args

@evverx
Copy link

evverx commented Dec 29, 2018

In #4542 (comment) @Dor1s expanded on why the OSS-Fuzz project uses CFLAGS to pass compiler and linker flags in their build scripts. What is going to happen to scripts like that? Are they going to stop working suddenly?

@jpakkane
Copy link
Member

jpakkane commented Jan 4, 2019

Needing to support things like that is probably why we can't ever truly get rid of the envvar support. 😿

Maybe we could instead parse the cflags settings and convert them to our internal option values?

@dcbaker
Copy link
Member Author

dcbaker commented Jan 4, 2019

that would be the other option I guess.

Alternatively we could add a meson level option like --read-env-vars, and make the description "you get to keep the pieces"

@keszybz
Copy link
Contributor

keszybz commented Jan 4, 2019

Yeah, --read-env-vars sounds like a reasonable compromise. This essentially makes the deprecation open-ended. Support for reading variables will have to be kept, so meson won't be simplified. But users will be nudged towards a better interface.

@evverx
Copy link

evverx commented Jan 4, 2019

I'm not sure I understand the point of adding --read-env-vars. It's still backward incompatible and every script where compiler options are supposed to be passed via CFLAGS has to be modified.

I mean, if I was willing to change every script I would have no problem with adding -Dc_args=... -Dc_link_args=... and it's unclear (at least to me) why anybody would need --read-env-vars.

@eli-schwartz
Copy link
Member

I'm extremely unclear on the rationalization for this change.

  • A thing which is not used on Windows doesn't matter, as it won't be read, but since meson purports to be a cross-platform build system I assume it is still worth keeping support for Unices.
  • Switching terminals and losing your CFLAGS is unlikely to happen, as they are usually set whenever initializing a build environment (either in e.g. bashrc or in sourced scripts or in the actual tools that e.g. distros use to run builds). Also
  • Reading environment variables only at initialization is not surprising behavior, since it is pretty standard for meta-build systems that generate the final build system.
  • Can you please point me to documentation where autotools tells people not to use CFLAGS? I'm unaware of this at all, but since autotools itself is the thing which reads those environment variables, it would be quite shocking if they also silently disapproved of them without telling users.
  • "They're basically inherited from the bad old days of plain makefiles" this is a strange assertion that is just that -- an empty assertion. (Also I object to your notion that plain makefiles are the bad old days. For simple things that don't need a pre-build configuration system, they're still the superior option. I recognize that as a competitor you have a vested interest in promoting meson instead...)

@eli-schwartz
Copy link
Member

eli-schwartz commented Jan 6, 2019

tell them to add -Dc_args=$CFLAGS -Dc_link_args=$LDFLAGS to their meson wrappers. I know for sure that gentoo, arch, and fedora have meson wrappers in their build infrastructure and don't call meson directly

As a distro packager for Arch, this script (arch-meson) makes me super uncomfortable and I refuse to use it in my own packages. I feel like a build system should not require elaborate sorcery just to get it to work on a basic level, and that wrapper script does not integrate into distro tooling on several levels (and moreover even if used, it would, well, require changing all packages to consistently use some non-default proxy tool).

The script you refer to also sets certain things that as a distro packager I feel are bad to set. It's opinionated, and that alone makes it unsuitable for mandatory status.

Please don't do this.

@keszybz
Copy link
Contributor

keszybz commented Jan 6, 2019

A thing which is not used on Windows doesn't matter, as it won't be read, but since meson purports to be a cross-platform build system I assume it is still worth keeping support for Unices.

The goal is to provide a syntax that works the same everywhere, reducing the difference in how meson is invoked on Linux and on Windows.

Can you please point me to documentation where autotools tells people not to use CFLAGS? I'm unaware of this at all, but since autotools itself is the thing which reads those environment variables, it would be quite shocking if they also silently disapproved of them without telling users.

Using the CFLAGS environment variable is discouraged. Instead, specifying CFLAGS= as a positional argument is recommended. Despite the very similar syntax, this is two completely different things. See https://stackoverflow.com/questions/13848154/passing-environment-variables-to-autoconfs-configure and https://unix.stackexchange.com/questions/290381/why-does-configure-take-variables-as-arguments for some good explanations and links.

Switching terminals and losing your CFLAGS is unlikely to happen, as they are usually set whenever initializing a build environment (either in e.g. bashrc or in sourced scripts or in the actual tools that e.g. distros use to run builds).

Well, maybe yes, maybe not. Even if you add some flags to .bashrc, and source the script in some terminal window to get the updated values, you might switch to some other terminal window and forget to do the reload there. With the non-visibility of environment variables, forgetting to pass the configuration is always a possibility. And if you run the build using a script, you always pass the configuration so you don't really care if it's passed as variables or arguments.

Reading environment variables only at initialization is not surprising behavior, since it is pretty standard for meta-build systems that generate the final build system.

Well, maybe for you it's obvious that environment variables should only be read once, but that's not how env vars behave in general. I can set various environment variables in my shell, and they will be passed down to any programs which are invoked, with the exception of some variables that influence the build system.

The great advantage of configuration passed as arguments to meson configure is that they can only be changed explicitly during the configuration phase. There is no syntax to pass those arguments to the build phase (ninja -C build -Dc_args=foo is a syntax error). So there is no need to explain that settings passed directly to the build command are ignored, and things are just more obvious.

Caching is also more straightforward: anything which is passed as arguments to meson configure shall be cached.

As a distro packager, this script makes me super uncomfortable and I refuse to use it in my own packages. I feel like a build system should not require elaborate sorcery just to get it to work on a basic level

Eh, I think you exaggerate a lot. Setting -Dc_args=... -Dld_args=... is hardly "elaborate sorcery".
Also, not that the rpm scriptlet quoted above is something that the (Fedora) distribution chooses to use, because we want to set those particular variables, and nothing forces you do this in your project.

@evverx
Copy link

evverx commented Jan 6, 2019

The goal is to provide a syntax that works the same everywhere, reducing the difference in how meson is invoked on Linux and on Windows.

It's possible to do that without breaking scripts relying on CC, CXX, CFLAGS and so on (good luck with deprecating CC and CXX, by the way :-)) by providing an additional option like --env-vars=[strict|i-know-what-i-am-doing] to people who think that meson should crash every time CFLAGS is set.

@evverx
Copy link

evverx commented Jan 6, 2019

Anyway, before I go, if progress can't be stopped I just wanted to say that it'd be nice if someone here who strongly believes that backward compatibility doesn't really matter at least went through all the projects from https://github.com/google/oss-fuzz/tree/master/projects and "just" set -Dc_args=... -Dld_args=... there so that they kept working. Plus, it shouldn't be that hard to find publicly available scripts on GitHub, GitLab and so on depending on environment variables and fix them without "elaborate sorcery". That would be a reasonable compromise I think :-)

@jpakkane
Copy link
Member

jpakkane commented Jan 6, 2019

As I mentioned above it is entirely possible that we can't remove support for envvars because a gazillion different things depend on them.

@evverx
Copy link

evverx commented Jan 6, 2019

@jpakkane as far as I understand, @dcbaker and @keszybz keep supporting the idea of deprecating environment variables and breaking everything along the way so my comments are mostly addressed to them. I should have mentioned them explicitly. Sorry about that.

@keszybz
Copy link
Contributor

keszybz commented Jan 6, 2019

There's a couple of meanings of "remove" being discussed here:

  1. really remove — only the new syntax is supported (-Dc_args=...)
  2. accept both the new syntax and old syntax (env vars)
  3. accept the old syntax with an explicit switch (--read-env-vars or --env-vars=i-know-what-i-am-doing or whatever)

I guess option 1 has been now shot down.

Option 2 is better than nothing, but doesn't protect against accidents.

I think 3 is reasonable compromise. It's true that a the parameter would have to be added to various projects using meson, but it's just a single line. In my experience adjustments to meson configuration are required pretty regularly when meson version changes, and this would be yet another such change. Meson community is still relatively small and such changes are easier now than they will be later on.

Option 3 could be implemented with a deprecation period: start accepting both -Dc_args=... and continue to accept CFLAGS=.... If both are specified -Dc_args= have higher priority. In the next release, if only $CFLAGS are specified, accept, and emit a warning unless --read-env-vars is used. In the next release after that, accept $CFLAGS only if --read-env-vars. This would drag things out, but allow smoother transition with the possibility to have a warning-less build that supports two consecutive versions of meson at the same time.

We should remember that the author of make kept the tabs-are-significant rule because he already had 15 users and it was too late to break compatibility.

@evverx
Copy link

evverx commented Jan 6, 2019

@keszybz

In my experience adjustments to meson configuration are required pretty regularly when meson version changes, and this would be yet another such change.

That's exactly why I don't consider meson a build system that can be used in environments less chaotic than experimental distributions. I kind of supported the choice the systemd project made (and if I recall correctly it was me who merged the PR) but I didn't expect it to be in this beta state for so long. But I digressed :-)

Option 3 could be implemented with a deprecation period

I doubt people look at https://oss-fuzz-build-logs.storage.googleapis.com/index.html regularly and care whether their builds are warning-less or not. I'm pretty sure the breakage will be noticed only after the deprecation period ends (that is when ClusterFuzz starts to complain that daily builds are broken).

Anyway, I already summed up what I think in #4664 (comment) so I don't think I can add anything else here. It's up to the meson project after all.

@keszybz
Copy link
Contributor

keszybz commented Jan 7, 2019

@evverx A grep over https://github.com/google/oss-fuzz for "meson" finds only four projects. Projects may install meson internally in build.sh, so there might be some more. Either way, it's not a big issue.

@evverx
Copy link

evverx commented Jan 7, 2019

@keszybz it isn't a big issue but it should be done and I'm kind of expecting that those who are going to break things on purpose have done their research and ready to fix what they are going to break (at least what is publicly available) instead of telling people to waste their time.

Meson community is still relatively small and such changes are easier now than they will be later on.

"meson" finds only four projects

Right. Let's keep it that way.

@nirbheek
Copy link
Member

nirbheek commented Jan 7, 2019

I've been watching this issue from the sidelines, and I agree with Jussi that it is very unlikely that we will ever remove support for reading env vars. CI usage won't be affected because that's an easy change to make, but the out-of-the-box user experience will be worse because users will expect env vars to be picked up similar to Autotools and grow frustrated when it doesn't work.

Hence, most likely we will make it a warning and add an option --ignore-env-vars that relies on the native file and command-line arguments for everything so that CI usage is more reliable and easier to debug.

@dcbaker
Copy link
Member Author

dcbaker commented Jan 7, 2019

I think that the consensus is that removing such env var support is never going to happen, so I'm going to go ahead and close this.

@dcbaker dcbaker closed this as completed Jan 7, 2019
@evverx
Copy link

evverx commented Jan 8, 2019

Thank you!

Interestingly, while I was complaining about environment variables and backward compatibility here, I completely missed an attempt to stop setting DBUS_SESSION_BUS_ADDRESS, which was unfortunately discovered only after systemd was released. Then the change was half reverted and it's still broken :-) I'm pretty sure there should be a name for when something can't be removed after it has been made publicly available.

Anyway, thanks again for preventing this kind of issues here!

@ePirat
Copy link
Contributor

ePirat commented Dec 20, 2019

Please re-open this issue, as for cross-compilation this is a big problem.

I agree that for native compilation env-vars should probably stay but for cross-compilation meson just does the "wrong" thing compared to nearly all other tools out there, as it will use the env vars for the native builds.
That means if I set CC env-var in a meta-build system to set the cross-compiler for autoconf and CMake, meson will treat that as the native compiler, even though it is the cross-compiler.

@ePirat
Copy link
Contributor

ePirat commented Dec 20, 2019

So my proposal would be, to remain compatible with existing use-cases, to add some flag like --ignore-env-vars to completely ignore env vars, like @nirbheek suggested.

This particular issue as caused already countless or super-hard to debug issues in VLC builds that it really would be great to have it fixed once and for all…

@jpakkane
Copy link
Member

Ignoring all envvars when a native file is used seems like a reasonable thing to do. The question is can we get away with it, it would be a breaking change.

@ePirat
Copy link
Contributor

ePirat commented Dec 20, 2019

@jpakkane I would like a explicit flag, so I can ignore them when cross-compiling even without a native file.

@xclaesse
Copy link
Member

+1 for having a flag in the native file saying "this file replace anything from env".

@eli-schwartz
Copy link
Member

There is a way to do this that works on all versions of meson, and doesn't require modifying meson at all:

env -i meson builddir/

@germandiagogomez
Copy link
Contributor

@eli-schwartz I think env -i can create problems with PATH? Not sure though.

@eli-schwartz
Copy link
Member

No, you just whitelist the environment variables you care about:

env -i PATH="$PATH" OTHERVAR="$OTHERVAR" meson builddir/

@marc-h38
Copy link
Contributor

This issue was about environment variables in meson's "input".

There are many other github issues about environment variables in meson "output", I mean environment variables in (some!) commands run by meson. The best starting point is probably #541 and then follow the links if needed.

bonzini added a commit to bonzini/qemu that referenced this issue Sep 30, 2020
Environment variables like CFLAGS are easy to accidentally change.  Meson
warns if that happens, but in a project with a lot of configuration that
is easy to lose.  It is also surprising behavior since meson caches -D
options and remembers those on reconfiguration (which we rely on,
since configure options become -D options).

By placing the user-provided CFLAGS, CXXFLAGS and LDFLAGS in the
cross file, we at least get consistent behavior.  These environment
variables are still ugly and not really recommended, but there are
distros that rely on them.  For the gory details, refer to
mesonbuild/meson#4664.

Reviewed-by: Richard Henderson <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
bonzini added a commit to bonzini/qemu that referenced this issue Oct 5, 2020
Environment variables like CFLAGS are easy to accidentally change.  Meson
warns if that happens, but in a project with a lot of configuration that
is easy to lose.  It is also surprising behavior since meson caches -D
options and remembers those on reconfiguration (which we rely on,
since configure options become -D options).

By placing the user-provided CFLAGS, CXXFLAGS and LDFLAGS in the
cross file, we at least get consistent behavior.  These environment
variables are still ugly and not really recommended, but there are
distros that rely on them.  For the gory details, refer to
mesonbuild/meson#4664.

Reviewed-by: Richard Henderson <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
Message-Id: <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
bonzini added a commit to bonzini/qemu that referenced this issue Oct 5, 2020
Environment variables like CFLAGS are easy to accidentally change.  Meson
warns if that happens, but in a project with a lot of configuration that
is easy to lose.  It is also surprising behavior since meson caches -D
options and remembers those on reconfiguration (which we rely on,
since configure options become -D options).

By placing the user-provided CFLAGS, CXXFLAGS and LDFLAGS in the
cross file, we at least get consistent behavior.  These environment
variables are still ugly and not really recommended, but there are
distros that rely on them.  For the gory details, refer to
mesonbuild/meson#4664.

Reviewed-by: Richard Henderson <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
Message-Id: <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
bonzini added a commit to bonzini/qemu that referenced this issue Oct 6, 2020
Environment variables like CFLAGS are easy to accidentally change.  Meson
warns if that happens, but in a project with a lot of configuration that
is easy to lose.  It is also surprising behavior since meson caches -D
options and remembers those on reconfiguration (which we rely on,
since configure options become -D options).

By placing the user-provided CFLAGS, CXXFLAGS and LDFLAGS in the
cross file, we at least get consistent behavior.  These environment
variables are still ugly and not really recommended, but there are
distros that rely on them.  For the gory details, refer to
mesonbuild/meson#4664.

Tested-by: Richard Henderson <[email protected]>
Reviewed-by: Richard Henderson <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
Message-Id: <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants