-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
sourmash: add v4.8.4 #40062
base: develop
Are you sure you want to change the base?
sourmash: add v4.8.4 #40062
Conversation
|
||
version("4.8.2", sha256="e0df78032e53ed88977445933ba3481dd10c7d3bd26d019511a6a4e6d7518475") | ||
version("4.8.4", sha256="43584c112c3310719771de175b89cba94f1c2d30b1aea46000eaf5a81efbae8a") |
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.
Can you sort from newest to oldest? This is just a customary style thing
|
||
depends_on("python@3.8:", type=("build", "run")) | ||
# build-only | ||
depends_on("py-maturin@0.14.13:0.14", type="build") | ||
depends_on("py-maturin@0.14.13:0.14", when="@4.8.2:", type="build") |
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.
depends_on("py-maturin@0.14.13:0.14", when="@4.8.2:", type="build") | |
depends_on("py-maturin@0.14.13:0.14", when="@4.8.2", type="build") |
Otherwise this constraint also gets applied to 4.8.4
depends_on("rust", type="build") | ||
# general | ||
depends_on("py-screed@1.1.2:1", type=("build", "run")) | ||
depends_on("py-cffi@1.14.0:", type=("build", "run")) | ||
depends_on("py-cffi@1.15.1:", when="@4.8.4:", type=("build", "run")) |
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 seems like 4.8.4 also supports 1.14+?
@@ -44,5 +50,3 @@ def install(self, spec, prefix): | |||
join_path("target", "release", "libsourmash.{}".format(lib_ext)), | |||
join_path(site_packages, "_lowlevel__lib.so"), | |||
) | |||
# patch invalid read mode | |||
filter_file(r"'(.*)'\), 130\)", r"'\1'))", join_path(site_packages, "_lowlevel.py")) |
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's the reason for this change? Is it no longer needed by any version or only by 4.8.4?
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.
As the author of the original spec, that filter was necessary for me to get the last version deployed on our Rocky8 Icelake cluster ... but, at the time, I was using a %gcc@13
stack. That was proving endlessly painful and I've since scrapped the whole lot and rebuilt everything with @12
.
Pulling the PR, can confirm that this line isn't needed for either version with my current stack, so I assume this was related to gcc@13
itself and/or to a difference in the dependencies pulled in by that stack in comparison to that of @12
. Both versions build and run a test through.
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 hit the same error when packaging sourmash for nixpkgs, and it also went away when upgrading to maturin>=1
.
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.
Yeah, that's the CFFI error I was getting back then as well. Took a while to track it down!
Tried to follow other PRs updating versions, but this is my first contributions, so let me know if I missed something =]