Environment variables defined in library_builders.sh are problematic

[native-api]

https://github.com/matthew-brett/multibuild/blob/devel/library_builders.sh defines numerous environment variables, globally. In MacOS, that includes compiler flags.

There are a least two problems here:

• Even though it declares it defines "library compilation flags", they also affect the project itself which is counterintuitive. (It took me a week to find out where "-arch i386 -arch x86_64" comes from 'cuz I never expected something called "library_builders.sh" to affect anything but library builds and blamed CMake logic or Travis.)
• The script's provided values are clearly designed to be overridable by presetting them in the environment. But in Docker environment, no user code is run before sourcing library_builders.sh, so it's impossible to override them there.

I'm not quite sure which course of action would be in line with the big design, so prefer to discuss this first.

[native-api]

The best ideas I currently have are:

• Define compilation flags in a function and only use it in library builds in a way that wouldn't alter the project's environment.
• In docker_build_wrap.sh, source some other user-provided file, e.g. "config_pre.sh", before sourcing library_builders.sh.

[ghost]

Multibuild builds fat OSX binaries by default. If you want, submit a PR moving/disabling that behavior with an environment variable.

[matthew-brett]

I think the user will usually want the build arteifacts on the include / library path - right? Hence the build flags being global.

It's not a bad idea adding something like env_vars.sh or config_pre.sh that can be executed before the source of the other stuff.

[native-api]

@xoviat The problem is not that I'm unhappy with the default choice, or can't override it (I just did in the linked commit). The problem is that neither I, nor @skvark ever saw this choice being made in the first place.

@matthew-brett Setting up build environment and building dependencies are two different stages of the build process. The latter one is even optional. The name "library_builders" suggests that it covers just the latter stage, not anything else.

I expected any logic that performs build environment setup to rather be together with the rest of the logic responsible for that step - i.e. somewhere like travis_<os>_steps.sh, or docker_build_wrap.sh (or a function in *_utils.sh that is called from there).

[ghost]

This is an OSS project. If you think that something should be different, submit a PR.

[native-api]

I'm not quite sure which course of action would be in line with the big design, so prefer to discuss this first.

I need to know what to submit before investing time into it.
At this point, I have enough feedback regarding the 2nd issue and can proceed with a PR for it.

[native-api]

Variants about relocating build environment setup code that crossed my mind. Each one has drawbacks.

For clarity, let's call each option, combined, as "Option X", and its parts that address the 1st and 2nd issues "Option X-1" and "X-2", correspondingly.

• Rename library_builders to something that would suggest it handles both dependencies and build environment. (Option 1-1)
  • no idea what
  • still violates separation of concerns?
  • still need two configuration files (Option 1-2)
• Split off that logic to yet another file (Option 2-1)
  • redundant? there are *_steps, *_utils etc already
  • still need two configuration files (Option 2-2 which is the same as 1-2)
• Move build environment setup code into a function (defined in _steps or _utils probably as it's OS-specific) and invoke it after config.sh is sourced. The user will thus be able to override it in the latter. (Option 3)
  • only need one config file
  • A function is not flexible enough for build environment configuration:
    • Build environment almost always needs be altered for a specific project, and most often, only altered partially.
    • Functions do not support partial reuse; neither is there an "inheritance" mechanism in Bash that would allow to chain a function with another with the same name.
  • very complicated, error-prone
    • The fact that the function is OS-specific adds yet more complication
    • all other logic of library_builders that has inputs also needs to be moved into a function -- yet more complication

[ghost]

We should use the third approach. Although functions don't support partial-reuse, it's what's previously been used for multibuild.

The fact that the function is OS-specific adds yet more complication

The function should be named the same for any OS, and should handle both OSX and Linux.

[native-api]

What about the "no inheritance" part? How are you going to call the stock function from the user-defined one with the same name? (And so on ifwhen e.g. OS-independent and OS-specific parts are separated.)

[ghost]

There's no inheritance in multibuild. See any of the other functions.

[native-api]

No other function needs to be inherited. The very few existing functions that are designed to be overridden (e.g. build_wheel) are trivial enough to be replaced entirely. ( "Configuration is by overriding the default build function, and defining a test function." Full stop.)

When I monkey-patched travis_linux_steps, I copy-pasted build_multilinux entirely and only deemed that acceptable because it's intended as a temporary solution until the fix goes into the release branch (i.e. the stock function is not expected to change in incompatible ways during that time).

[ghost]

So, theoretically on the multibuild side:

setup_environment
if is_function "post_setup_environment" post_setup_environment  # Potentially user-defined

And in config.sh:

function post_setup_environment {
    :
}

[native-api]

Then you need an entry point before and between each step :)

To be honest, there is a way to rename a function in Bash (so you can invoke it from the overriding one). And it's not pretty. (You also need to come up with a unique identifier for the rename.)

[ghost]

That's correct. You can add entry points where you think they should go.

[native-api]

This makes three already: before setup_environment, after init_library_builders (that's where library-related envvars would go), and between them. And once we add one, removing it would break backward compatibility.

[ghost]

Yes, of course.

[native-api]

There's yet another option that evaded me when writing https://github.com/matthew-brett/multibuild/issues/116#issuecomment-357372450.

• If you remember, the need for config_pre or an entry point before library_builders is because library_builders has a lot of envvar inputs and there's no way to bring them into Docker from the host environment.
  • So, if the user can make a list of additional variables to propagate, this specific need will vanish.
    • The implementation can use a Bash array to accumulate additional docker run args.
• This won't allow to define anything other than envvars. But nothing else needs to be defined at this point atm 'cuz library_builders doesn't have any other inputs (e.g. it doesn't call anything external upon import).

This will eliminate the need for the 1st entry point (however it's implemented).

The 2nd entry point isn't really needed atm to begin with 'cuz init_library_builders logic doesn't use anything defined by setup_environment on import.

And the 3rd point is already covered by config.sh.

So, with this, we can go the 1st/2nd option minus the 2nd config file. I also came up with the proposed names -- build_env+library_builders.sh and build_env.sh, correspondingly.

Besides, this is arguably a more intuitive solution 'cuz the normal expectation is for the initially set envvars to persist throughout the build.

[matthew-brett]

@native-api - I think you're proposing something like

ENV_VAR_NAMES=(FOO BAR BAZ)

in .travis.yml, where these variables would be copied across the the docker container with suitable -e FOO=$FOO type lines in the build_multilinux function?

The alternative I was proposing initially was to have an env_vars.sh file that would have:

FOO="my_foo_contents"
BAR="my_bar_contents"

etc, and which would be sourced from within the docker container - and by the OSX builds - before sourcing the other .sh files. The advantage of this is only that it's pretty easy to explain, in that it avoids a step of indirection. Also, you can put platform specific logic in there to deal with OSX / Linux differences. What do you think?

I'm not sure what you mean by "And the 3rd point is already covered by config.sh." - could you explain?

[ghost]

So the env_vars.sh seems to be the same idea as config_pre.sh -- I guess that's okay after all?

[matthew-brett]

It's OK with me - yes. Is that OK with you @native-api ?

[native-api]

Well, the fact that in config.sh, I cannot use the variables defined earlier in the build process was surprising enough to warrant a comment ( native-api/opencv-python@de9c286 ). Other than that (nitpick), this is the simplest solution of all, so fine by me, too (and, conveniently enough, has a PR ready ;-) ). Also note that it only addresses issue 2, so we still need to decide if anything needs to be done about the location of the build env setup code.

[native-api]

On issue 1, the names in https://github.com/matthew-brett/multibuild/issues/116#issuecomment-357821839 and the already listed options is all I could come up with.

Due to this being quite a memorable experience, I personally "know" about that library_builders' little trick by now (sure as hell I do...).

So, if you have qualms about it, we can close this issue and wait for another poor sap to step on this landmine and try to come up with something better... (this can very well be me again after several months).

[matthew-brett]

Issue 1 is that it's not obvious that "library_builders.sh" configures the build environment for all later steps.

How about having a separate file called configure_builds.sh that is sourced from library_builders.sh, and suitably commented?

[native-api]

@matthew-brett That's the option 2-1 earlier. I came up with build_env.sh name earlier, too (not insisting on it though). Split the logic off, or rename the original file, okay either way.

[native-api]

A dedicated configure_build.sh also opened an opportunity to consolidate build flags set by other files in one place. Combined with the fact the name for the rename is still rather ugly, op. 2-1 looks preferrable to me now.