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

Recipe using experimental cache: re-executes main build script #1051

Open
h-vetinari opened this issue Sep 9, 2024 · 26 comments
Open

Recipe using experimental cache: re-executes main build script #1051

h-vetinari opened this issue Sep 9, 2024 · 26 comments

Comments

@h-vetinari
Copy link

Translating zlib to rattler-build, I need to use cache:. However, after hacking around a bunch of other issues, it seems that build.sh gets re-executed -- and then fails, because it's not meant to do that.

In particular, I see the following in the logs

 │ │ + cmake --build .
 │ │ [1/41] Building C object CMakeFiles/zlib.dir/compress.c.o
 │ │ [2/41] Building C object CMakeFiles/zlib.dir/adler32.c.o
 │ │ [...]
 │ │ -- Installing: $PREFIX/lib/pkgconfig/zlib.pc
 │ │
 │ ╰─────────────────── (took 15 seconds) │
 │ ╭─ Resolving environment
 │ │ [...]
 │ ╰─────────────────── (took 5 seconds)
 │
 │ ╭─ Running build script
 │ │ [...]
 │ │ + set -ex
 │ │ + export 'CFLAGS=-march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem $PREFIX/inclu
 │ │ de -fdebug-prefix-map=/home/conda/feedstock_root/build_artifacts/bld/rattler-build_libzlib_1725865147/work=/usr/local/src/conda/libzlib-1.3.1 -fdebug-prefi
 │ │ x-map=$PREFIX=/usr/local/src/conda-prefix -fPIC'
 │ │ + CFLAGS='-march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem $PREFIX/include -fde
 │ │ bug-prefix-map=/home/conda/feedstock_root/build_artifacts/bld/rattler-build_libzlib_1725865147/work=/usr/local/src/conda/libzlib-1.3.1 -fdebug-prefix-map=$
 │ │ PREFIX=/usr/local/src/conda-prefix -fPIC'
 │ │ + export 'CXXFLAGS= -fPIC'
 │ │ + CXXFLAGS=' -fPIC'
 │ │ + mkdir -p /home/conda/recipe_root/ignored
 │ │ + mkdir build
 │ │ mkdir: cannot create directory ‘build’: File exists
 │ │
 │ ╰─────────────────── (took 0 seconds)

The fact that $SRC_DIR/build already exists is because build.sh has already been executed.

@wolfv
Copy link
Member

wolfv commented Sep 9, 2024

I just checked out the PR. Awesome, and thanks for trying!!
I am wondering what you want to execute in the cache phase? Because right now, you don't specify any script in the cache section?

@h-vetinari
Copy link
Author

That's just my lack of familarity with the new format (plus conda-recipe-manager not handling that case yet). I wanted to run build.sh / bld.bat as before. I've now specified that manually, let's see what happens. It does already get picked up though even without specifying anything.

@h-vetinari
Copy link
Author

No change with

 cache:
   requirements:
     build:
       - cmake
       - ninja
       - ${{ compiler('c') }}
       - ${{ stdlib('c') }}
+  build:
+    script:
+      - if: linux
+        then: build.sh
+        else: build.bat

i.e. something still re-executes build.sh, even though it isn't specified for any other output. Maybe the "magic" build.sh file name causes that?

@wolfv
Copy link
Member

wolfv commented Sep 9, 2024

Yes, it does :)

@wolfv
Copy link
Member

wolfv commented Sep 9, 2024

Just call the file build_cache.sh/bat and use build: script: build_cache. Rattler-build figures out the extension itself.

@wolfv
Copy link
Member

wolfv commented Sep 9, 2024

Every output (incl. the cache) is independent and follows the same rules about e.g. build scripts and so far we've copied the implicit behavior of conda-build.

@h-vetinari
Copy link
Author

and so far we've copied the implicit behavior of conda-build.

One difference seems to be that the run-exports of subpackages do get applied within a given recipe? Conda-build doesn't do that... FWIW, I think it'd be better to apply them, but just checking.

@h-vetinari
Copy link
Author

Just call the file build_cache.sh/bat and use build: script: build_cache. Rattler-build figures out the extension itself.

OK, did that now, now running into issues with the file glob (I think).

Logs of testing zlib look like:

 │ │ │ ✔ Successfully updated the environment
 │ │ │ Testing commands:
 │ │ │ + test -f $PREFIX/lib/libz.a
 │ │ │ + test -f $PREFIX/lib/libz.so
 │ │ │ + test -f $PREFIX/include/zlib.h
 │ │ │
 │ │ ╰─────────────────── (took 4 seconds)
 │ │
 │ ╰─────────────────── (took 4 seconds)
 │
 ╰─────────────────── (took 9 seconds)
 × error Error building package: failed to run test: Script failed with status Some(1).
 × error Work directory: "/home/conda/feedstock_root/build_artifacts/bld/rattler-build_zlib_1725876013/work/test/run/etc/conda/test-files/zlib/0"
 × error To debug the build, run it manually in the work directory (execute the `./conda_build.sh` or `conda_build.bat` script)
Error:   × failed to run test: Script failed with status Some(1).
  │ Work directory: "/home/conda/feedstock_root/build_artifacts/bld/rattler-
  │ build_zlib_1725876013/work/test/run/etc/conda/test-files/zlib/0"
  │ To debug the build, run it manually in the work directory (execute the `./
  │ conda_build.sh` or `conda_build.bat` script)

AFAICT (more below), this actually fails for test -f $PREFIX/include/zlib.h, but that is really not clear from the logs. The only way I could tell is that

 │ │ Files in package:
 │ │   - etc/conda/test-files/zlib/0/test_compile_flags.bat
 │ │   - etc/conda/test-files/zlib/0/test_compile_flags.c
 │ │   - lib/libz.a
 │ │   - lib/libz.so
 │ │   - info/about.json    <-- should contain `include/...` before `info/`

So that means that the following file glob doesn't work:

    build:
      files:
        - if: linux
          then: lib/libz.so
        - if: osx
          then: lib/libz.dylib
        - if: unix
          then:
            - include
            - lib/pkgconfig
            - lib/libz.a
          else:
            - Library/include
            - Library/share
            - Library/lib/zlib.lib
            - Library/lib/zdll.lib
            - Library/lib/z.lib
            - Library/lib/zlibstatic.lib
            - Library/lib/pkgconfig

What would be the correct way to specify this?

@wolfv
Copy link
Member

wolfv commented Sep 9, 2024

Hmm, not sure how you mean it exactly. We apply run exports from the cache & from each output. In the console log you can see the source of the run export.

We thought about appending the run export from the cache only to the "first" output in a chain of outputs (e.g. cache -> libzlib -> zlib). If they are (exact) linked, it wouldn't make a difference to apply the export to both outputs. But that is not implemented yet and also functionally should not make a difference.

You can also ignore run exports in the cache, or in (each) output.

@wolfv
Copy link
Member

wolfv commented Sep 9, 2024

Can you try include/?

@h-vetinari
Copy link
Author

h-vetinari commented Sep 9, 2024

Hmm, not sure how you mean it exactly.

I mean that for conda-build, the run-exports of libzlib do not get applied to zlib, even though it depends on it:

      host:
        - {{ pin_subpackage('libzlib', exact=True) }}

Similarly, the global build step does not apply run-exports to the outputs, at least not in general. That's why many recipes (including zlib's meta.yaml) have something like

      build:
        - {{ compiler('c') }}
        - {{ stdlib('c') }}

for the outputs, even though no script is being run - it's purely to pick up the right run-exports.

I don't feel strongly about which default is better, but I'd like to have consistent and predictable rules for when they do apply. If the rule is that cache: applies REs to outputs, I'm fine with that. If the rule is that a subpackage's RE (like libzlib here) applies also to other outputs in that recipe (like zlib here), then I'm fine with that. It's just something we'll need to keep in mind while translating recipes.

@h-vetinari
Copy link
Author

Can you try include/?

That does work, though it's stricter than what conda-build does. Is it necessary to require the trailing / for folders...? 🤔

@wolfv
Copy link
Member

wolfv commented Sep 9, 2024

Define necessary :D I think if you do ls include you won't see the contents of the folder. So I think in terms of "glob" it's more appropriate, and I also like things to be stricter. But we can totally debate this in another issue.

@h-vetinari
Copy link
Author

I think if you do ls include you won't see the contents of the folder.

I see no difference (on my CLI) between ls folder and ls folder/ (both on linux & on win). 🤷

I also like things to be stricter. But we can totally debate this in another issue.

I think we have a breakage-budget for the transition that we shouldn't overstretch. The more subtle behaviour changes accumulate, the harder it's going to be to pull off. I think in case of the REs it probably makes sense to pull the trigger on a clean-up. OTOH, for folder-globs I don't see the benefit in terms of clarity gained compared to all the recipe's we'll break -- perhaps silently, because why duplicate in the tests what's already specified under files:, right? -- and then have to remember to look out for and fix.

@wolfv
Copy link
Member

wolfv commented Sep 24, 2024

I think we can close this, right? Or do you see anything that needs to be done with regards to the implicit build.sh file?

@h-vetinari
Copy link
Author

Or do you see anything that needs to be done with regards to the implicit build.sh file?

Whatever RB does with build.sh in multi-output recipes seems to diverge from what CB does. I think it's going to be a stumbling block.

To my understanding, CB (with build.sh in multi-output builds) will only run build.sh for the global stage, and not for other outputs. Looking at the state of conda-forge/zlib-feedstock@f60ba78, RB will try to run build.sh per output even if no script: is defined for that output. I don't think that's a good idea.

I think we're mostly on the same page with moving away from magic behaviour, and I like the idea of having to specify the build script also for the global/cache phase (like we discussed above), but it shouldn't be executed IMO for script:-less outputs.

@wolfv
Copy link
Member

wolfv commented Sep 24, 2024

rattler-build's behavior is relatively straight-forward: if there is a build.sh or build.bat file, use it. All outputs (incl. the cache output) are treated equally.

I would argue that it's bad practice to have a build.sh for multi-output recipes (if that build.sh file is not doing special things like being conditional on the PKG_NAME, which some of them do).

@h-vetinari
Copy link
Author

I would argue that it's bad practice to have a build.sh for multi-output recipes

Sure, but that's the only way to do it currently. Why do you need to break it? Why do you need to execute build.sh for all outputs implicitly (==magically), rather than those outputs opting into having an explicit script:? It doesn't even seem necessary for consistency. You could discourage use of build.sh but still not break existing recipes by executing it per default for all outputs.

@wolfv
Copy link
Member

wolfv commented Sep 24, 2024

I am fine with removing the default altogether!

@h-vetinari
Copy link
Author

I am fine with removing the default altogether!

Same. So I'd be fine with:

  • cache: requiring script: build.sh to translate existing multi-output recipes
  • other outputs not executing build.sh at all unless the specify their own per-output script: to do-it.

@wolfv
Copy link
Member

wolfv commented Sep 24, 2024

I mean globally, single- or multi-output recipes not having a default build.sh behavior.

@h-vetinari
Copy link
Author

I mean globally, single- or multi-output recipes not having a default build.sh behavior.

That's consistent/compatible with what I said.

@olivier-roussel
Copy link
Contributor

olivier-roussel commented Oct 9, 2024

Just to point out I've been trapped the same way by the re-execution of default build script (i.e. build.sh) for each output in the case we use this name for cache build scripts, which I found a bit counter-intuitive. I also think removing the default build.sh behavior in general (single or multi ouputs) might avoid some common mistakes.

@wolfv
Copy link
Member

wolfv commented Oct 10, 2024

@h-vetinari @olivier-roussel I think it would be less breaking if we change the behavior of the cache output to not use the build.sh by default.

What do you think about that?

@olivier-roussel
Copy link
Contributor

It makes sense, I agree with that.

@h-vetinari
Copy link
Author

Yeah. I think we could go so far as just doing away with a default script execution at all (at least if there's more than one output, but then the only consistent thing would be to do it everywhere). Just let users specify the name for the build script of each output.

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

3 participants