Skip to content
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

Re-export mlir_sys #583

Open
cospectrum opened this issue Sep 1, 2024 · 10 comments · May be fixed by #588
Open

Re-export mlir_sys #583

cospectrum opened this issue Sep 1, 2024 · 10 comments · May be fixed by #588

Comments

@cospectrum
Copy link

Add pub use mlir_sys;, since mlir_sys types appear in the public api.
Inkwell re-exports llvm_sys for the same reason.
https://www.lurklurk.org/effective-rust/re-export.html

@raviqqe
Copy link
Member

raviqqe commented Sep 3, 2024

Cann't we use mlir_sys adding it as a dependency?

@cospectrum
Copy link
Author

cospectrum commented Sep 3, 2024

A user can depend on mlir_sys only if it has a semver compatible version that matches the version in melior toml, which is not always possible and which imposes a limitation on the user

@raviqqe
Copy link
Member

raviqqe commented Sep 3, 2024

Hmmm, but isn't the situation the same as pub use mlir_sys; because it's exposed from a melior crate?

@cospectrum
Copy link
Author

mlir_sys exported from melior crate will have the version that melior has in its toml. 2 different mlir_sys

@raviqqe
Copy link
Member

raviqqe commented Sep 3, 2024

Why do you want to use the old version of mlir-sys?

@cospectrum
Copy link
Author

cospectrum commented Sep 3, 2024

Different reasons. Example of the problem: If a new semver version of mlir_sys comes out tomorrow, users start a project by running cargo add melior mlir_sys or they run cargo update, their project won't compile, and they come here to complain. Seen this in practice

@cospectrum
Copy link
Author

Or if there is another crate in the project that depends on mlir_sys and publicly uses types, its version does not match melior, and the crate does not export its version of mlir_sys either. Then I simply cannot use melior and that crate at the same time.

@raviqqe
Copy link
Member

raviqqe commented Sep 3, 2024

If a new semver version of mlir_sys comes out tomorrow, users start a project by running cargo add melior mlir_sys or they run cargo update, their project won't compile, and they come here to complain. Seen this in practice

For this one, they can just specify a minor version like mlir-sys = "0.2" in their Cargo.toml.

Or if there is another crate in the project that depends on mlir_sys and publicly uses types, its version does not match melior, and the crate does not export its version of mlir_sys either. Then I simply cannot use melior and that crate at the same time.

For this one, they can use a package attribute for the second mlir-sys dependency. For example,

mlir-sys = "0.2"
mlir-sys1 = { version = "0.1", package = "mlir-sys" }

WDYT?

@cospectrum
Copy link
Author

Solution for the first problem is correct, but may require some work if the project uses cargo update check in in ci.
Solution for the second problem may require a complex rewrite of the entire codebase. And grep + sed does not always work. I would simply prefer to use melior::mlir_sys from the beginning.

@cospectrum
Copy link
Author

It's not difficult for melior, just 1 line in lib.rs 🥺

@cospectrum cospectrum linked a pull request Sep 13, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants