Skip to content

Fix Rust Releases - #556

Open
AkshatRai07 wants to merge 7 commits into
masterfrom
fix/rust-releases
Open

Fix Rust Releases#556
AkshatRai07 wants to merge 7 commits into
masterfrom
fix/rust-releases

Conversation

@AkshatRai07

@AkshatRai07 AkshatRai07 commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Closes #555

In this PR we,

  • Fix MacOS linking errors.
  • Remove staticlib generation.
  • Make eko versioning dynamic, hence changing the .whl name.

Copilot AI review requested due to automatic review settings July 22, 2026 11:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts the Rust C-API build/test setup to address macOS dynamic linking issues and to stop producing a staticlib artifact during releases.

Changes:

  • Configure the C-API crate to build only as a cdylib (no staticlib).
  • Add a macOS-specific post-link step in the C-API test runner to rewrite an absolute dylib install name to use @rpath.
  • Update the local build-capi task command used to generate the C-API distribution.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
pyproject.toml Updates the build-capi task command for producing the C-API dist.
crates/ekore_capi/tests/run_tests.sh Adds a macOS-specific install-name rewrite to make test binaries find the dylib via @rpath.
crates/ekore_capi/Cargo.toml Removes staticlib from the crate’s crate-type list to stop generating static libraries.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pyproject.toml
Comment thread crates/ekore_capi/tests/run_tests.sh Outdated
@AkshatRai07

AkshatRai07 commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

Doubt: Should Rust, C-API, and Python API tests be run on all OS and Architecture combinations which we will use when publishing?

Suggestion: Please try changing the permission of PYPI_TOKEN, and run the entire Maturin workflow, only then merge this PR. Else we will have to wait for v0.15.6.

@AkshatRai07

AkshatRai07 commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

There was one more issue, if you see the artifacts of maturin.yml, the version of eko-rs is 0.0.0. I checked locally and changing the version to be dynamic worked.

@scarlehoff

Copy link
Copy Markdown
Member

Rust, C-API, and Python API tests be run on all OS and Architecture combinations which we will use when publishing?

A lot of researchers do use macOS locally and all clusters are linux so the minimal set would be mac-arm and linux-x64

@scarlehoff scarlehoff left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lgtm, but one question, was dynamic = ["version"] a necessary change for this to work?

(can always be rolled back I guess)

@AkshatRai07

AkshatRai07 commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

I tried changing bump-versions.py to make it change the version in pyproject.toml too, but that didn't work. Setting dynamic = ["version"] makes it use the version of the Cargo.toml which is already bumped by bump-versions.py. Also, it is just a one line change and it works so I guess it is the best way here.

@felixhekhorn felixhekhorn added bug Something isn't working rust Rust extension related gsoc26 labels Aug 5, 2026
@felixhekhorn

Copy link
Copy Markdown
Collaborator

Suggestion: Please try changing the permission of PYPI_TOKEN, and run the entire Maturin workflow, only then merge this PR. Else we will have to wait for v0.15.6.

@scarlehoff eko (and so the token I think) is owned by the "N3PDF" user on PyPI - to which I have no longer access, since it needs 2FA now (I have only the password). I hope you still have access? 🙈 if so, can you please take care of this?

@felixhekhorn

felixhekhorn commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Concerning the location of the CAPI files: is this triggered by skipping "Publish release" https://github.com/NNPDF/eko/actions/runs/29902592251/job/89195933659 and will get fixed once we have all artifacts (which were failing before)?

@felixhekhorn

Copy link
Copy Markdown
Collaborator

Since we are fixing #537 here: can I please add one more unrelated thing? After having said that, I'm comparing https://nnpdf.github.io/pineappl/docs/installation.html#building-the-capi-from-source to https://docs.rs/ekore_capi/0.15.5/ekore_capi/#building--consuming . I think in PineAPPL pointing out how to install Rust or how to install cargo-c or to check out the repo is too verbose (and to be expected from the user), but I like the ${prefix} handling there. Instead in EKO pointing out how to compile a C program is too verbose (and to be expected from the user).

I suggest to do the following:

  • tell the user they need cargo-c (is the comment there about staticlib still relevant after 2c4160b? )
  • give the cargo command with ${prefix}
  • tell that files x,y,z have been created
  • remind the user they may need to adjust their environment vars
  • tell the user they now can just use pkg-config in the usual way

@felixhekhorn

Copy link
Copy Markdown
Collaborator

one more glitch from #537: the links here

//! C-language interface for [`ekore`], the crate providing the anomalous dimensions and

and here
//! computed quantities, and the [ekore docs](https://docs.rs/ekore/latest/ekore) for the

are effectively the same, but different in practice - I suggest to use always the former. The second version is also used below
//! [ekore docs](https://docs.rs/ekore/latest/ekore).

@felixhekhorn

Copy link
Copy Markdown
Collaborator

Add one more glitch from #550 (sorry for spamming this with unrelated stuff 🙈 ):

for (let el of shorts) replaceAbbrev(el);

needs an additional

let tables = document.getElementsByClassName("item-table");
for (let el of tables) replaceAbbrev(el);

to make the abbreviations also work, e.g., also on the start page function list (etc.)

@felixhekhorn felixhekhorn mentioned this pull request Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working gsoc26 rust Rust extension related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix rust releases

4 participants