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

[rc] Add llvm-tools-<maj> on windows as well #285

Open
wants to merge 7 commits into
base: rc
Choose a base branch
from

Conversation

h-vetinari
Copy link
Member

No description provided.

@conda-forge-webservices
Copy link

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.

recipe/install_llvm.bat Outdated Show resolved Hide resolved
@conda-forge-webservices
Copy link

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.

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

For recipe/meta.yaml:

  • No valid build backend found for Python recipe for package lit using pip. Python recipes using pip need to explicitly specify a build backend in the host section. If your recipe has built with only pip in the host section in the past, you likely should add setuptools to the host section of your recipe.

@conda-forge-webservices
Copy link

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.

@h-vetinari
Copy link
Member Author

h-vetinari commented Aug 24, 2024

Or couldn't we compile our own wrapper-exe (per binary) that just calls the versioned one from C

I just tried that now. It's not as light as a symlink, but waaaay lighter than copying. Each binary takes ~12kb; for 80 binaries in llvm-tools, that amounts to less than 1 MB, which I think is negligible (certainly compared to 300MB if we copied).

590f8d1 is obviously not meant for merge/review (logs with that commit), but it shows that the forwarder is robust enough to replace %LIBRARY_BIN%\cmake during the build and have all the rest work out.

Would appreciate your thoughts @isuruf

@h-vetinari h-vetinari changed the title Add llvm-tools-<maj> on windows as well [rc] Add llvm-tools-<maj> on windows as well Aug 25, 2024
@h-vetinari h-vetinari marked this pull request as ready for review August 25, 2024 22:19
@@ -0,0 +1,103 @@
// adapted from example in MSFT docs, see
// https://learn.microsoft.com/en-us/windows/win32/procthread/creating-processes
Copy link
Member

Choose a reason for hiding this comment

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

What's the license on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

MIT, see here, which covers the code in doc source

Copy link
Member Author

Choose a reason for hiding this comment

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

Any other concerns besides the license?

Copy link
Member Author

Choose a reason for hiding this comment

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

Gentle ping on this @isuruf

Copy link
Member Author

Choose a reason for hiding this comment

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

Another ping here... I'm aware this is a trade-off between consistency across platforms and extra complexity (though we hopefully shouldn't have to touch it going forward; might also consider breaking this out into something reusable wherever we have versioned binaries). I don't have a clear answer what is better, so I'd appreciate your input @isuruf.

Copy link
Member

Choose a reason for hiding this comment

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

Can we just use symlink-exe-substitute-feedstock ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like that's built to symlink an exe into another folder, not to a differently-named file in the same folder (it also doesn't use the relative path logic). That said, the approach could probably be extended to name the target symlink, and it's cleaner because it doesn't duplicate the 12kb per link (at the cost of introducing an extra run-dependency).

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.

2 participants