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

Switch to rattler-build #83

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Conversation

h-vetinari
Copy link
Member

This PR is not immediately meant for merging, but for doing a logical separation of different changes on the way to exploratory emscripten support. Builds on #82

Still needs a bunch of fixes in our infra.

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/recipe.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/recipe.yaml:

This is a rattler-build recipe and not yet lintable. We are working on it!

recipe/recipe.yaml Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Member Author

There's a subtle behaviour change in how %LIBRARY_LIB% existed during the global build stage before, but doesn't anymore. No big deal, but triggered a failed copy here, c.f. 70e47af

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/recipe.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/recipe.yaml:

This is a v1 recipe and not yet lintable. We are working on it!

@wolfv
Copy link
Member

wolfv commented Sep 26, 2024

I've re-triggered some builds to see if rattler-build 0.22.0 changes anything :)

@h-vetinari
Copy link
Member Author

Interesting - on windows, cmake doesn't seem to be found; it's definitely there though, and also works before the translation to RB.

 │ │ Resolving build environment:
 │ │   Platform: win-64 [__win=0=0, __archspec=1=x86_64_v4]
 │ │   Channels: 
 │ │    - file:///D:/bld/
 │ │    - conda-forge
 │ │   Specs:
 │ │    - cmake
 │ │    - ninja
 │ │    - vs2019_win-64
 │ │    - vs_win-64
 │ │ [...]
 │ │ 'cmake' is not recognized as an internal or external command,
 │ │ operable program or batch file.

@wolfv
Copy link
Member

wolfv commented Sep 26, 2024

We can see that the osx version is detected as 10.16 now, too:

 │ │ Resolving host environment:
 │ │   Platform: osx-64 [__unix=0=0, __osx=10.16=0, __archspec=1=skylake]
 │ │   Channels: 
 │ │    - file:///Users/runner/miniforge3/conda-bld/
 │ │    - conda-forge
 │ │   Specs:
 │ │    - __osx >=11.0

Which breaks the tests, since they require 11.0.

@wolfv
Copy link
Member

wolfv commented Sep 26, 2024

Strange because jolt-physics seems fine (also CMake) conda-forge/jolt-physics-feedstock#3

@h-vetinari
Copy link
Member Author

Strange because jolt-physics seems fine (also CMake) conda-forge/jolt-physics-feedstock#3

Perhaps another little 🐞 hiding in cache:? 🤔

@h-vetinari
Copy link
Member Author

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you but ran into some issues. Please check the output logs of the GitHub actions workflow below for more details. You can also ping conda-forge/core for further assistance or you can try [rerendering locally](https://conda-forge.org/docs/maintainer/updating_pkgs.html#rerendering-with-conda-smithy-locally.

The following suggestions might help debug any issues:

  • Is the recipe/{meta.yaml,recipe.yaml} file valid?
  • If there is a recipe/conda-build-config.yaml file in the feedstock make sure that it is compatible with the current global pinnnings.
  • Is the fork used for this PR on an organization or user GitHub account? Automated rerendering via the webservices admin bot only works for user GitHub accounts.

This message was generated by GitHub actions workflow run https://github.com/conda-forge-webservices/actions/runs/11139428595.

@h-vetinari
Copy link
Member Author

FYI @wolfv @beckermr
The admin bot didn't manage to rerender here, but manually it worked (modulo changes necessary for conda-forge/conda-smithy#2057)

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Oct 2, 2024

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe/recipe.yaml) and found some lint.

Here's what I've got...

For recipe/recipe.yaml:

  • The top level meta key cache is unexpected

@h-vetinari
Copy link
Member Author

but manually it worked

Nevermind, that's almost certainly because I monkey-patched my local install (xref prefix-dev/rattler-build-conda-compat#56).

@h-vetinari
Copy link
Member Author

@wolfv, cmake isn't found in any of the builds -- at least now the error is consistent across platforms I guess 😅

Looks like a cache: problem to me. Do you want me to open an issue in rattler-build?

@wolfv
Copy link
Member

wolfv commented Oct 2, 2024

An issue with a small reproducer would be great :)

@h-vetinari
Copy link
Member Author

Now it should be pretty minimal. ;-)

And now it's slightly less minimal, but actually reproduces the problem 😅

@h-vetinari
Copy link
Member Author

@wolfv, thanks for the new rattler-build release! Looking better now (had to disable interactions with ci-setup, which cannot handle --experimental yet either). There was a segfault on linux-aarch64 that went away on restart, but otherwise unix looks fine.

On win-64 the tests fail with some unclear cause

 │ │ │ Installing test environment
 │ │ │  Successfully updated the test environment
 │ │ │ Testing commands:
 │ │ │ (base) C:\Users\VSSADM~1\AppData\Local\Temp\.tmpdO7zDL>IF "" == "" (call C:\Users\VSSADM~1\AppData\Local\Temp\.tmpdO7zDL\build_env.bat ) 
 │ │ │
 │ │ ╰─────────────────── (took 0 seconds)
 │ │
 │ ╰─────────────────── (took 0 seconds)
 │
 ╰─────────────────── (took 71 seconds)
 × error Error building package: failed to run test: Script failed with status Some(1).
 × error Work directory: "C:\\Users\\VSSADM~1\\AppData\\Local\\Temp\\.tmpdO7zDL"
 × 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).

and on win-arm64 it crashes as before, but now we see why

thread 'main' panicked at src\cache.rs:281:9:
assertion `left == right` failed: Prefixes must have the same length: "D:\\bld\\bld\\rattler-build_libzlib_1727940322\\h_env" != "D:\\bld\\bld\\rattler-build_libzlib-wapi_1727940322\\h_env"
  left: 49
 right: 54

Difference is related to the outputs somehow, i.e. rattler-build_libzlib{,-wapi}_1727940322 in the environment path.

@wolfv
Copy link
Member

wolfv commented Oct 3, 2024

Yes, awesome. Thanks for retrying! On win-64 it fails during test-execution if I see correctly. The win-arm64 issue is curious. Will have to investigate (but looks fixable!).

@h-vetinari
Copy link
Member Author

On win-64 it fails during test-execution if I see correctly.

Is this how test failures generally look, or are we running into specific case here? If it's due to a failing test (and not some problem in the supporting infra), I'd want to have much more information about which line or assertion failed, rather than Some(1).

The win-arm64 issue is curious. Will have to investigate (but looks fixable!).

Cool, thanks!

@wolfv
Copy link
Member

wolfv commented Oct 3, 2024

@h-vetinari if you have an idea about a different way of invoking cmd.exe that would be welcome :) Maybe there are some parameters that will print the line that is executed etc. (like bash).

@h-vetinari
Copy link
Member Author

@h-vetinari if you have an idea about a different way of invoking cmd.exe that would be welcome :) Maybe there are some parameters that will print the line that is executed etc. (like bash).

Just execute the individual instructions from the test section line-by-line?

@wolfv
Copy link
Member

wolfv commented Oct 3, 2024

I don't think that's the best solution as that's pretty different to other script executions. If it's in a list then it would be possible (otherwise we'd have to parse the script and split it on logical boundaries). I'll try to see if there is a way to enable a better way to trace script execution.

Btw. for package content tests, you can use the new package_content test that produce much better output and run insanely fast (because they only check the paths.json file) :)

Example: https://github.com/conda-forge/jolt-physics-feedstock/blob/be2718fdc372f0bd5cb33950751eb7eaf092a591/recipe/recipe.yaml#L54-L59

@h-vetinari
Copy link
Member Author

I don't think that's the best solution as that's pretty different to other script executions.

I think this is what conda-build does (but don't know); I'm not talking about the content of some .cmd or .bat script by the way, just the stuff that's under:

test:
  commands:
    - ...

should be executed one-by-one (and that principle can even apply to all platforms), because you really want to know at which line things failed, and not just "in the test section".

Btw. for package content tests, you can use the new package_content test that produce much better output

Sure, that'll cover most cases I hope. But wherever we run things that cannot be translated to other tests under commands:, those lines should be executed separately. They're already a list when they come out of the yaml, so you'd just have to iterate through.

@h-vetinari
Copy link
Member Author

h-vetinari commented Oct 3, 2024

you can use the new package_content test

It also doesn't work here because we have a lib that doesn't follow the standard naming pattern (z.lib, zlib.lib, zlib.dll, zlibstatic.lib), and more importantly, the DLL and the import library are in separate outputs, and also there's a DLL in a non-standard location (not %LIBRARY_BIN%).

@wolfv
Copy link
Member

wolfv commented Oct 3, 2024

You can still use globs to check etc. make them platform independent and so on. All these cases are easily covered.

@h-vetinari
Copy link
Member Author

Thanks, I guess using files: directly will work for presence tests.

You can still use globs to check etc. make them platform independent and so on. All these cases are easily covered.

However, we often want to specifically check the absence of something, mostly that there's no static library. I don't see how that's covered yet.

@wolfv
Copy link
Member

wolfv commented Oct 3, 2024

Yes, we already have the design for negative globs as well :) prefix-dev/rattler-build#915

@h-vetinari
Copy link
Member Author

I'll try to switch to the package_content tests, but regardless of how much can be converted, having no output for

test:
  commands:
    - ...

is a complete dealbreaker from my POV. I don't even see the inconsistency you mentioned with other script executions (well, those that are defined in a yaml list at least) - in those cases it's equally important to know which line failed.

@wolfv
Copy link
Member

wolfv commented Oct 3, 2024

Definitely! If you have a simple repro I can debug or can come up with something myself

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

Successfully merging this pull request may close these issues.

4 participants