-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
Fix clash of names with upstream function #1471
Conversation
60de5a8
to
cfd5c4a
Compare
Codecov Report
@@ Coverage Diff @@
## main #1471 +/- ##
==========================================
+ Coverage 70.04% 70.05% +0.01%
==========================================
Files 377 377
Lines 57772 57773 +1
Branches 19406 19406
==========================================
+ Hits 40465 40473 +8
+ Misses 14746 14739 -7
Partials 2561 2561
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
123839d
to
f8e3b1d
Compare
91b17d8
to
5a8fa90
Compare
While this is ready for a review, I am leaving this in draft mode until HighFive 2.7.1 is released. Update: while HighFive 2.7.1 is released (and is expected to fully resolve all issues), the conda packages aren’t generated/uploaded yet. |
887dac0
to
18051c5
Compare
18051c5
to
3cccca0
Compare
3cccca0
to
5904986
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.
Thanks for finding the solution to this, @ischoegl -- the CI error messages definitely did not make it clear what was going wrong. I had some suggestions that I think will simplify the configuration checks a bit.
dc55fb3
to
c910c42
Compare
I guess I need to give a shoutout to folks over at the HighFive project, who were extremely helpful and responsive in the debugging effort. Other than that, I rebased and addressed three of the four suggestions. For the last one, I no longer rewrite the file, but am not sure how to move things out of |
12d87d8
to
8be1467
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.
Thanks, @ischoegl, this looks good to me. Just one small simplification left, I think.
Also use system installation by default.
HighFive 2.7.0 introduces a new create_enum_boolean function, which clashes with an existing local function in Storage.cpp.
8be1467
to
97a6c11
Compare
@speth ... could you re-approve? (fixing the last item dismissed your approval) |
HighFive 2.7.0 introduces
h5py
-style support for booleans, which is close to identical with what was already implemented inStorage.cpp
. HighFive uses the exact same name for a newcreate_enum_boolean
function, which clashes with an existing local function inStorage.cpp
, causing CI failures when conda packageshighfive
with versions 2.7.0 and newer are used.The resolution is to use the full name for older versions of HighFive (2.7.0 and older), while using HighFive's implementation for boolean support for HighFive 2.7.1 and newer (this avoids a change of their API from 2.7.0 to 2.7.1). For the time being, the git submodule remains on 2.6.2, while conda will grab the newest package available.
Other updates:
See BlueBrain/HighFive#713 and BlueBrain/HighFive#724
Checklist
scons build
&scons test
) and unit tests address code coverage