-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
add bolt #22204
base: main
Are you sure you want to change the base?
add bolt #22204
Conversation
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 ( Here's what I've got... For recipes/bolt:
For recipes/bolt:
|
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 ( I do have some suggestions for making it better though... For recipes/bolt:
|
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 ( |
Looks like the pyston people have been packaging bolt for their conda channel already, but in the full build variant (i.e. using |
I don't really understand why |
Hi friend! We really, really, really appreciate that you have taken the time to make a PR on In an effort to maintain this repository and increase the signal-to-noise for open PRs, the maintainers of If you'd like to keep it open, please comment/push and we will be happy to oblige! Note that very old PRs will likely need to be rebased on Cheers and thank you for contributing to this community effort! |
not stale |
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 ( |
Updated based on discussion in conda-forge/llvmdev-feedstock#276; essentially using the state from conda-forge/llvmdev-feedstock#279, minus obviously the bits that package llvmdev |
This will fail on windows due to some missing components in the azure images; on the llvmdev feedstock we're working around this with conda-forge/llvmdev-feedstock@221c9f3 (and I can do the same for bolt once this is merged). If the situation persists, I guess we'll have to teach smithy how to do that. |
@isuruf, would you like to review? It should be a pretty simple recipe now. Another advantage of splitting this off is that we won't miss any other files that bolt might install (and I did miss one binary - |
{% endfor %} | ||
# only on linux-64 | ||
- test -f $PREFIX/lib/libbolt_rt_hugify.a # [linux64] | ||
- test -f $PREFIX/lib/libbolt_rt_instr.a # [linux64] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also check that the cmake files are here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only way to do that currently is to clobber
lib/cmake/llvm/LLVM-Config.cmake
lib/cmake/llvm/LLVMConfig.cmake
lib/cmake/llvm/LLVMConfigVersion.cmake
lib/cmake/llvm/LLVMExports-release.cmake
lib/cmake/llvm/LLVMExports.cmake
from llvmdev, unfortunately. But I guess better than nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bolt doesn't have different cmake files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, it's built as part of llvm. I guess we could ask for separate CMake files in the "standalone" upstream PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change your opinion somehow, or are you fine with the always_include_files:
(0e96961)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isuruf, could you let me know how you'd like to proceed with the CMake metadata here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make libbolt-devel
depend on llvmdev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'm planning to merge this once CI passes (well, on unix at least; the rest I'll fix on the feedstock).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't. We require reviews from everyone including core team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then please don't drip-feed change requests. If there's been two rounds of review and no more open comments are left, it's not unreasonable to think that things are wrapped up.
Ugh, I don't know why
I've double-checked the paths, and they're there
|
Co-authored-by: Marcel Bargull <[email protected]>
mkdir %LIBRARY_LIB%\cmake\llvm | ||
move .\temp_prefix\lib\LLVMBOLT*.lib %LIBRARY_LIB% | ||
REM copy CMake metadata | ||
move .\temp_prefix\lib\cmake\llvm %LIBRARY_LIB%\cmake\llvm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about include files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had been wondering about that too, but there's nothing I can find. The only occurrences of include/bolt
in the logs (example) are during the build, not during installation. I've now done a regex-search (include/.*bolt
) as well, and it only finds false positives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's anything that goes beyond what's in llvmdev
, it's now being installed (4f17e53)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are no include files, then we might as well drop libbolt-devel
as users can't use those libraries (not easily anyways).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know sufficiently well how bolt works, but it's conceivable to me that bolt the binary needs those libraries to actually transform a binary (perhaps embedding some symbols from the support libraries along the way).
It seemed natural to split off the libraries, but if you want, I can just stuff everything into bolt
itself.
- test -f $PREFIX/lib/libbolt_rt_hugify.a # [linux64] | ||
- test -f $PREFIX/lib/libbolt_rt_instr.a # [linux64] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two should be in bolt
as they are used at runtime of bolt
. The others are only needed if bolt is used as a library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure (between the two open threads) what you want me to do now. Here you say
The others are only needed if bolt is used as a library.
But in the other you say
If there are no include files, then we might as well drop
libbolt-devel
as users can't use those libraries
I understood that we'll move libbolt_rt_{hugify,instr}
to bolt
. But for the rest, do we want to:
- ship them as
libbolt-devel
- ship them as part of
bolt
- not ship them at all until a use-case comes up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 is preferred. 1 if not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I found the bolt headers: https://github.com/llvm/llvm-project/tree/main/bolt/include/bolt; no idea why they're not getting installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I guess we can restrict this to linux until anything changes upstream w.r.t. llvm/llvm-project#72205
Fixes conda-forge/llvmdev-feedstock#147
Towards #13019
Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details).