-
Notifications
You must be signed in to change notification settings - Fork 44
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
ipp 2021.10 build fix #1583
ipp 2021.10 build fix #1583
Conversation
d3c5dcf
to
e9767eb
Compare
e9767eb
to
3301aa0
Compare
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.
Just had a quick look at the changes to see if this could solve our problems in other repos, and noticed some requests for reverting headers etc.
I will re-review fully when the actions are working 😄
499c62c
to
cb926ab
Compare
There's a lot of unrelated changes in this so it's hard to check the impact. Some builds are failing on GHA and Jenkins but not all so it would be good to know what's different between these. Was the ipps.h being added to the C++ code the only fix here? I don't like including conda requirements of '>' as this is impossible to restrict in the future if there are issues. If we need to pin a particular version we should. The build scripts are conveniences for creating environments are conveniences for us and I think having more options is better than less. We Can support the environment file for users? This is something that we need to sort out and simplify in the developer/contribution guide. |
The GHA and Jenkins issue is the high priority thing here. And we need to locally run |
cb926ab
to
9525d0b
Compare
moved to separate PRs; please re-review :)
can you post links (URLs) to failing builds?
Please explain "impossible to restrict in the future if there are issues". Given that '>' is used a lot already before this PR, please do open a separate GH issue to discuss why you think they should all be removed. |
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.
It looks good to me. Although Intel may break again compatibility in the future, I don't think having >=
is detrimental. At least we know that it won't work with version lower than 2021.10.
-DLIBRARY_INC=$CONDA_PREFIX/include \ | ||
-DOPENMP_LIBRARIES=${CONDA_PREFIX}/lib \ | ||
-DOPENMP_INCLUDES=${CONDA_PREFIX}/include \ | ||
-DOPENMP_LIBRARIES=${CONDA_PREFIX}/lib | ||
else | ||
echo "something else"; | ||
-DCMAKE_INSTALL_PREFIX=$PREFIX |
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.
did you try on Darwin?
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.
no and I'm also uncertain about whether it may need e.g. dylib
. I think I'll add a GHA macos test in a follow-up. wdyt?
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.
The windows builds all worked. If Linux worked if happy to merge.
You should add yourself here: https://github.com/TomographicImaging/CIL/blob/master/NOTICE.txt |
Please update the Changelog https://github.com/TomographicImaging/CIL/blob/master/CHANGELOG.md |
Describe your changes
ipps.h
has been moved toipp/ipps.h
ipp.h
already (indirectly) includes the above anywayrecipe/build.sh
include dirsDescribe any testing you have performed
Please add any demo scripts to CIL-Demos/misc/
Link relevant issues
TODO in follow-ups:
scripts/*.sh
tidy dev scripts #1590sphinx
build on recentpython
versions update sphinx #1597conda-forge
CI matrices #1598Checklist when you are ready to request a review
I have added docstrings in line with the guidance in the developer guideI have implemented unit tests that cover any new or modified functionalityContribution Notes
Please read and adhere to the developer guide and local patterns and conventions.