prefix-dev / rattler-build

rattler-build is a universal package builder for Windows, macOS and Linux
https://prefix-dev.github.io/rattler-build
BSD 3-Clause "New" or "Revised" License
210 stars 45 forks source link

BUG: Variables from variants not populated in build scripts #1056

Closed h-vetinari closed 4 weeks ago

h-vetinari commented 1 month ago

While trying to build zlib for emscripten, I'm setting the following in CBC

cross_target_platform:  # [linux64]
  - linux-64            # [linux64]
  - emscripten-wasm32   # [linux64]

and have a branch in the build script à la:

if [[ ${cross_target_platform} == emscripten-* ]]; then
    # ...
fi

However, when building this with rattler-build, the variables end up not being set, i.e. the log shows

 │ │ + [[ '' == emscripten-* ]]

where the '' part should not be empty.

That's despite this being picked up for the various outputs - but perhaps not for cache:? 🤔

 │ Build variant: zlib-1.3.1-hcf6bdf7_2
 │ 
 │ ╭───────────────────────┬───────────────────╮
 │ │ Variant               ┆ Version           │
 │ ╞═══════════════════════╪═══════════════════╡
 │ │ c_compiler            ┆ gcc               │
 │ │ c_compiler_version    ┆ 13                │
 │ │ c_stdlib              ┆ sysroot           │
 │ │ c_stdlib_version      ┆ 2.17              │
 │ │ channel_targets       ┆ conda-forge main  │
 │ │ cross_target_platform ┆ emscripten-wasm32 │     <--- see here
 │ │ libzlib               ┆ 1.3.1 h7eb9a27_2  │
 │ │ target_platform       ┆ linux-64          │
 │ ╰───────────────────────┴───────────────────╯
wolfv commented 1 month ago

Yes, you'll have to use the variable in the cache inside the YAML somehow. I think there is a force use variables option you could try, for example.

wolfv commented 1 month ago

Or you can pass it as an env var in the script context. We don't scan build scripts for used variables.

h-vetinari commented 1 month ago

We don't scan build scripts for used variables.

It's not a question of scanning the build scripts, which I agree is not necessary, or even a good idea. But the variables coming from the variant config should be defined during the build scripts, unless there's a very strong reason not to do this...?

A huge amount of recipes depend on this, it'll be really painful to work around this IMO.

wolfv commented 1 month ago

It's not the way conda build works. Variables have to be used to influence the variant.

wolfv commented 1 month ago

You can have a very long variant configuration file but only a subset will apply to your recipe.

Conda forge just pre processes all of this in conda-smithy.

h-vetinari commented 1 month ago

Variables have to be used to influence the variant.

I understand that; but it's enough to use it in the recipe

It's not the way conda build works.

conda-build does populate variants (picked up from the recipe) also for the build script. Which is my point, because a lot of recipes rely on things that are defined in the .ci_support/*.yaml files (however they get picked up by conda-build as "used") to be available during the build script.

h-vetinari commented 1 month ago

In the case of https://github.com/conda-forge/zlib-feedstock/pull/84, a rerender with rattler-build populates cross_target_plaform in .ci_support/linux_64_cross_target_platformemscripten-wasm32.yaml as expected (because it appears in the recipe; i.e. scanning the build script is not necessary).

The only thing to do AFAIU is to pre-populate the environment variables present during the build scripts with whatever is in the respective .ci_support/*.yaml config -- conda-build does it, recipes rely on it, and so it'd be mega-painful to transition recipes at scale if rattler-build won't do the same.

wolfv commented 1 month ago

But that would be wrong since we don't create a variant for it...

You need to use the variant key in the output to make it appear in the build script. Otherwise the behavior would be super broken.

wolfv commented 1 month ago

For example, you could have multiple outputs. One depends on python and one does not.

Only the one that depends on python will build the python variant packages. The python variant value will not be set during the build for the non-python output (even though it appears in the variant config). The value would be garbage anyways.

Does that make sense?

h-vetinari commented 1 month ago

The value would be garbage anyways.

Does that make sense?

I understand that aspect I think. But most of those cases are completely benign. Populating an env var for python_impl and then not using it for a certain output is not much of a problem. The only problem would be if separate outputs have contradictory values, but that's already not really compatible with populating any single value in .ci_support/*.yaml.

I really think we need to match conda-build here, because not picking up certain flags in build scripts can lead to arbitrarily painful behaviour. The build failing is the easiest case, but it could lead to subtly broken behaviour because (for example) a platform-specific branch that was added after debugging a previous problem suddenly doesn't trigger anymore. Again, we could warn on these things and help people improve their recipes by making things explicit, but IMO we cannot realistically transition to v1 recipes in any reasonable timeframe if there are such fundamental and subtle behaviour changes.

Beyond that point, how would one specify that cross_target_platform gets populated correctly in the build script of a given output?

wolfv commented 1 month ago

The most obvious way would be to do:

build:
  script:
     file: foo.sh
     env:
       CROSS_TARGET_PLATFORM: ${{ cross_target_platform }}

In the appropriate output.

The other way would be to use

build:
  variant:
     use_keys: ["cross_target_platform"]

In conda-build this probably works because they do analyze the build.sh/bld.bat file with a regex.

h-vetinari commented 1 month ago

The other way would be to use

build:
  variant:
     use_keys: ["cross_target_platform"]

Thanks. I tried this, but it still doesn't set the value properly:

+ [[ '' == emscripten-* ]]

(logs)

wolfv commented 1 month ago

And the other way works? :)

h-vetinari commented 1 month ago

And the other way works? :)

Not tested yet, but I already don't like it, because it creates yet another indirection and divergence from conda-build. 😑

h-vetinari commented 1 month ago

So first I did:

    script:
      # rattler-build figures out the {.sh,.bat} extensions
      - build_global
+     # see https://github.com/prefix-dev/rattler-build/issues/1056
+     env:
+       CROSS_TARGET_PLATFORM: ${{ cross_target_platform }}

but that doesn't work because then one needs to shift

    script:
      - build_global

to

    script:
      file: build_global

This is pretty bad dev experience IMO (I had two other failed attempts in between, because of invisible list-vs-scalar expectations). Why not make file:[^1] mandatory rather than let people fall into a trap once they need to add an env: or secrets:?

PS. It does work in the end, but I don't think it's a feasible to do a migration with this.

[^1]: or rather, either file: or content:

wolfv commented 1 month ago

What you typed is also not valid YAML though (mixing list & map). I don't see the trap there, sorry (any editor would put red squiggly lines below that).

h-vetinari commented 1 month ago

What you typed is also not valid YAML though (mixing list & map)

Sure (and I did try with - env too), but most parts of the current recipe infrastructure are built to work additively in terms of lines. Having to change the way the script-name is passed because we're adding env: is surprising/annoying.

wolfv commented 1 month ago

OK, I think I initially understood your request slightly wrongly. The bug you have discovered is about making the variant key/values available as environment vars in the build script. That is something that seems to be broken in rattler-build indeed. I'll double check if that is also a problem for a regular single-output build.

We will definitely fix that! :)

h-vetinari commented 1 month ago

The bug you have discovered is about making the variant key/values available as environment vars in the build script.

👍

Sorry I didn't manage to communicate this more clearly.

wolfv commented 4 weeks ago

Fixed by #1088