Fix c-blosc2 #1773
Merged
Fix c-blosc2 #1773
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Need |
This was referenced Apr 5, 2024
Merged
|
@meteorcloudy @fmeum Can you please add the |
fmeum
added
presubmit-auto-run
|
Thanks! I confirm that this fixes all of the zfp linker issues similar to the excerpt pasted below: |
Vertexwahn
force-pushed
the
fix-c-blosc2
branch
from
022a346
to
0cc5242
Compare
|
I have no clue why the tests are failing. The patch file contains several cc_test targets. Seems like the patch is not applied for the test run... |
meteorcloudy
requested changes
Apr 12, 2024
Vertexwahn
force-pushed
the
fix-c-blosc2
branch
from
0cc5242
to
cc3c5c4
Compare
|
Finally green... ;) |
|
@fmeum @meteorcloudy Ready to merge! |
fmeum
previously approved these changes
Apr 12, 2024
|
@meteorcloudy Seems that I need a review from you... |
auto-merge was automatically disabled
April 13, 2024 18:34
Head branch was pushed to by a user without write access
Vertexwahn
force-pushed
the
fix-c-blosc2
branch
from
1817abd
to
bf71e9c
Compare
bazel-io
dismissed
fmeum’s stale review
Require module maintainers' approval for newly pushed changes.
Vertexwahn
force-pushed
the
fix-c-blosc2
branch
from
bf71e9c
to
a380a9f
Compare
meteorcloudy
requested changes
Apr 15, 2024
Vertexwahn
force-pushed
the
fix-c-blosc2
branch
from
a380a9f
to
d05f140
Compare
Vertexwahn
force-pushed
the
fix-c-blosc2
branch
from
b631307
to
a5e396d
Compare
meteorcloudy
approved these changes
Apr 16, 2024
aiuto
pushed a commit
to aiuto/bazel-central-registry
that referenced
this pull request
Jun 3, 2024
AcademySoftwareFoundation/openexr#1604 revealed that the c-blosc2 module still has some linker issues. This PR fixes those issues. This PR is an improvement of the c-blosc2 model at version 2.12.0. In detail, missing c files were added to the c-blosc2 target. Also, textual_hdrs were reworked. Besides this cc_tests targets were introduced to make sure the whole thing works across different systems. The defines `HAVE_PLUGINS` and `HAVE_ZSTD` were added. Without the define `HAVE_PLUGINS` zfp plugin is not initialized (which is required by OpenEXR). @phaedon Maybe this fixes also some issues on your side ;). You can create a PR and add yourself as a maintainer to this module if you like. This way you will get notified if "improvements" are made or at least it gives you a chance to veto changes ;). Furthermore, the PR policy was changed in a way that a positive review of a maintainer can lead under some circumstances to a successful merge without the need to get a review of one of the "admins/repo owners"
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
AcademySoftwareFoundation/openexr#1604 revealed that the c-blosc2 module still has some linker issues. This PR fixes those issues. This PR is an improvement of the c-blosc2 model at version 2.12.0.
In detail, missing c files were added to the c-blosc2 target. Also, textual_hdrs were reworked. Besides this cc_tests targets were introduced to make sure the whole thing works across different systems.
The defines
HAVE_PLUGINSandHAVE_ZSTDwere added. Without the defineHAVE_PLUGINSzfp plugin is not initialized (which is required by OpenEXR).@phaedon Maybe this fixes also some issues on your side ;). You can create a PR and add yourself as a maintainer to this module if you like. This way you will get notified if "improvements" are made or at least it gives you a chance to veto changes ;). Furthermore, the PR policy was changed in a way that a positive review of a maintainer can lead under some circumstances to a successful merge without the need to get a review of one of the "admins/repo owners"