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

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

Closed
h-vetinari opened this issue Sep 10, 2024 · 20 comments
Closed

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

h-vetinari opened this issue Sep 10, 2024 · 20 comments

Comments

@h-vetinari
Copy link

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
Copy link
Member

wolfv commented Sep 10, 2024

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
Copy link
Member

wolfv commented Sep 10, 2024

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

@h-vetinari
Copy link
Author

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
Copy link
Member

wolfv commented Sep 10, 2024

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

@wolfv
Copy link
Member

wolfv commented Sep 10, 2024

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
Copy link
Author

h-vetinari commented Sep 10, 2024

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
Copy link
Author

h-vetinari commented Sep 10, 2024

In the case of conda-forge/zlib-feedstock#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
Copy link
Member

wolfv commented Sep 10, 2024

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
Copy link
Member

wolfv commented Sep 10, 2024

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
Copy link
Author

h-vetinari commented Sep 10, 2024

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
Copy link
Member

wolfv commented Sep 10, 2024

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
Copy link
Author

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
Copy link
Member

wolfv commented Sep 10, 2024

And the other way works? :)

@h-vetinari
Copy link
Author

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
Copy link
Author

h-vetinari commented Sep 10, 2024

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.

Footnotes

  1. or rather, either file: or content:

@wolfv
Copy link
Member

wolfv commented Sep 10, 2024

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
Copy link
Author

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
Copy link
Member

wolfv commented Sep 10, 2024

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
Copy link
Author

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 wolfv changed the title BUG: Variables from variants not populated in build scripts (for cache:?) BUG: Variables from variants not populated in build scripts Sep 11, 2024
@wolfv
Copy link
Member

wolfv commented Oct 7, 2024

Fixed by #1088

@wolfv wolfv closed this as completed Oct 7, 2024
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

2 participants