Skip to content

Conversation

@carterbox
Copy link
Member

@carterbox carterbox commented Dec 12, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Fixes #1

  • File paths on Windows need to be $PREFIX/Library.
  • cmake targets contain incorrect paths which needs to be patched for Windows
  • address lints from linter

The build number is not bumped because the linux builds are unchanged.

@carterbox carterbox requested review from a team and kvoronin as code owners December 12, 2024 20:05
@conda-forge-admin
Copy link
Contributor

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/meta.yaml) and found some lint.

Here's what I've got...

For recipe/meta.yaml:

  • ❌ Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [31, 32, 33, 34, 35]

For recipe/meta.yaml:

  • ℹ️ It looks like the 'libcudss-commlayer-nccl' output doesn't have any tests.
  • ℹ️ It looks like the 'libcudss-commlayer-mpi' output doesn't have any tests.
  • ℹ️ The recipe is not parsable by parser conda-souschef (grayskull). Your recipe may not receive automatic updates and/or may not be compatible with conda-forge's infrastructure. Please check the logs for more information and ensure your recipe can be parsed.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12303936871. Examine the logs at this URL for more detail.

@conda-forge-admin
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/meta.yaml) and found it was in an excellent condition.

files:
- lib/libcudss.so.* # [linux]
- bin/cudss64_0.dll # [win64]
- Library/bin/cudss64_0.dll # [win]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work on Windows ARM?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference between win and win64 selectors is "any windows" vs "64-bit windows", so the real question is "Does this work on 32-bit Windows?" The answer is "No", but that's moot because conda-forge doesn't have separate 32-bit/64-bit Windows build variants.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also no builds for cudss on Windows ARM, so we don't know what the filename would be for those builds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historically that would have been true

In the near future win64 will be used to distinguish Windows x86_64 from Windows arm64: conda-forge/conda-forge-pinning-feedstock#5440

Hence the question about Windows ARM

It is ok that it is unsupported atm. Just thought we were trying to clarify between these newer Windows architectures

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are they patching conda-build too? Because according to the docs, win64 doesn't know about x86 vs arm.

https://docs.conda.io/projects/conda-build/en/latest/resources/define-metadata.html#preprocessing-selectors

Copy link
Member

@jakirkham jakirkham Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conda already knows about it: conda/conda#11778

Most likely we will need to compose OS & arch selectors using this sort of approach

For now win64 has been used as shorthand for win and x86_64. Similar to how linux64 is already used

@carterbox carterbox merged commit 899f8f1 into conda-forge:main Dec 12, 2024
6 checks passed
@carterbox carterbox deleted the windows-cmake-paths branch December 12, 2024 20:57
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.

Windows build failure on main

4 participants