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

Add CI workflow to publish crates on tag/release #14

Merged
merged 5 commits into from
Jan 21, 2024

Conversation

mtreinish
Copy link
Member

This commit adds a new github actions work to publish the package's crates to crates.io. This new CI job run cargo publish on the respective crates in the correct order to publish the entire workspace.

This commit adds a new github actions work to publish the package's
crates to crates.io. This new CI job run cargo publish on the respective
crates in the correct order to publish the entire workspace.
@jlapeyre
Copy link
Collaborator

jlapeyre commented Jan 11, 2024

Thanks!
I'm trying to understand what the names of the published crates will be. Should we give them uniform prefixes? Maybe it's already in there? I see that the modules have been renamed.

Also, as discussed. I'll remove the pyo3 related crate and edit this PR.

@mtreinish
Copy link
Member Author

See #13 which prepares the packages for publishing. This just is adding the CI jobs to do the publishing when we're ready to tag 0.0.1.

Comment on lines 2 to 15
name: Release openqasm3_parser
on:
push:
tags:
- '*'

jobs:
publish_lexer:
name: Publish oq3_lexer crate
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
crate: [oq3_lexer, oq3_parser, oq3_semantics, oq3_sourcegen, oq3_syntax, oq3_source_file]
Copy link
Member

Choose a reason for hiding this comment

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

  • name: openqasm3_parser
  • jobs.publish_lexer.name: oq3_lexer crate
  • matrix: [...]

three different things haha

Comment on lines +17 to +25
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
- name: Run cargo publish
run: |
cd crates/${{ matrix.crate }}
cargo publish
env:
CARGO_REGISTRY_TOKEN: ${{ secrets.CARGO_REGISTRY_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

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

Do the crates get verified crates.io-side at all? If so, we might have to force the order of them to some topological iteration through them to get them uploaded.

Seems a shame that there's no workspace-wide cargo publish.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh that's a good point, yeah we'll need to do this manually in topological order otherwise cargo's validation will error when the dependencies aren't resolvable with crates.io. You get an error like:

error: failed to prepare local package for uploading

Caused by:
  no matching package named `oq3_lexer` found
  location searched: registry `crates-io`

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is true rust-lang/cargo#1169 (comment) , then there is no problem publishing out of order, as long as the crates have been published once in the correct order (say manually).

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a ton of discussion around this and many tools available. This tool looks to me (fwiw) to be the most serious and perhaps relevant.
https://github.com/crate-ci/cargo-release

@jlapeyre
Copy link
Collaborator

jlapeyre commented Jan 11, 2024

I originally had prefixed package names in Cargo.toml with oq3_. I worked this way for quite a while, but found it inconvenient. I was happier after removing the prefixes. So I wonder if it's possible to change just the package name, or whatever is used for retrieving from crates.io.

This appears to be what rust analyzer has done. For example the crate in rust analyzer directory hir-def exports module hir and is in package ra_ap_hir_def.

And the parser crate is somehow published as ra_ap_parser. If this is using only standard rust and cargo features, then I'd prefer to use this route. But they may be using custom publishing scripts. I want to avoid that.

@mtreinish
Copy link
Member Author

The discussion around the name should probably be in #13 which actually renames things. In general they can be different but I would strongly argue against it. Besides the obvious consistency issues with potentially have 3 different names (crate, module, and directory) for the same thing, the obvious conflict concerns with having such a generic name like parser apply equally to the package name on crates.io as they do for the actual module name. If you published a package as oq3_parser but still had use parser in the code that would be as likely to conflict with something else that was also being used in the same package's development. I did originally leave the directory names the same in #13 but @jakelishman correctly pointed out we should be consistent in how we refer to things otherwise it'll just be confusing.

@jakelishman
Copy link
Member

From the perspective of using it outside a single application (like building on top of it in Qiskit), it's better to have the prefixes, and that aligns closer with how it's actually packaged. For example, see how PyO3 (a library, rather than an application) works and packages itself.

Within itself, you can always use x as y; to rename if you need, whereas if we do that outside the library, we're going through two+ logical renames.

@jlapeyre
Copy link
Collaborator

Another thing that's becoming clear. If find it convenient for developing to sometimes organize code on the level of crates. For various reasons. For example, but not only, the code can easily be built and tested separately.

However when installing or using crates, it looks like the namespace is flat. So each crate in a workspace has to be installed as a separate crate in ~/.cargo and it has to have a globally unique name. Is this correct?

If so, its worth considering things like moving the code in source_file into semantics.

@jlapeyre
Copy link
Collaborator

The discussion around the name should probably be in #13 which actually renames things.

This is correct. I was busily editing my last comment, then saw that I had two response comments in the mean time.

@jakelishman
Copy link
Member

Yeah, since we have to hit a flat namespace, the oq3_ prefix give-or-take serves the same purpose as if we could have published all of the crates in this workspace under a oq3:: prefix.

@jlapeyre
Copy link
Collaborator

we should be consistent in how we refer to things

We shouldn't have any gratuitous or accidental inconsistency. But it's common in the PL world to have a regular rule for deriving names. Eg. package 'the-packageprovidesThePackage`.

Yeah, since we have to hit a flat namespace, the oq3_ prefix give-or-take serves the same purpose as if we could have published all of the crates in this workspace under a oq3:: prefix.

I don't follow. The flat namespace is present at the level of crates.io ? But it is possible to publish crates under a prefix, for example oq3::?

Within itself, you can always use x as y; to rename if you need, whereas if we do that outside the library, we're going through two+ logical renames.

Doing use x as y internally probably takes care of most of my objections to using the prefix. Doing it once at the top of the file is not so bad. And I am pretty sure the semantics of rust makes this strictly equivalent to using a different name (this is not the case in all languages)

Or we can do what r-a does:

#![cfg_attr(feature = "in-rust-tree", feature(rustc_private))]

#[cfg(not(feature = "in-rust-tree"))]
extern crate ra_ap_rustc_lexer as rustc_lexer;
#[cfg(feature = "in-rust-tree")]
extern crate rustc_lexer;

Beautiful! =(

@jlapeyre
Copy link
Collaborator

jlapeyre commented Jan 13, 2024

Continuing discussing here what should have been discussed in #13: I committed #13 without changing the naming scheme used therein. For example oq3_lexer is used everywhere, never lexer.

@jlapeyre
Copy link
Collaborator

@mtreinish and @jakelishman This PR is no longer blocked by anything.

I spent an hour or so investigating how to publish, but did not see an obvious best way.

Apparently publishing is problematic enough that there are several tools available to assist.

Many issues are discussed. For example, after a crate is published, it takes an indeterminate amount of time before it is available to satisfy a dependency of a subsequently published crate. It seems there may be an automatic sleep interval inserted into some publishing tools. And I saw some talk of an adjustable period because no single period was satisfactory.

However, someone claimed that versions are not checked when determining if dependencies are satisfied.
Unfortunately, I am unable to find this claim again or any other evidence that it is true or not. If I understand correctly, this means that if we can register and upload these crates just once, then we should be able to upload them in any order and at any time in the future (assuming we don't trigger some security mechanism for avoiding ddos or filling the archive with garbage, unlikely with three or four small crates)

@mtreinish
Copy link
Member Author

There is specifically an issue with the first publishing of this though. The name needs to exist on crates.io at least and in the case of the first push none of the crates exist yet. We can manually create a topological sorting of the dependency tree though in the matrix list https://github.com/Qiskit/openqasm3_parser/pull/14/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R15 and that will just work if as @jakelishman says we serialize the jobs. I forgot about the parallel execution when I originally opened the PR, although I consciously tried to do the sorting when I originally wrote the PR (but I might have done the sort incorrectly).

@jlapeyre
Copy link
Collaborator

Right. I meant we can figure out the dependence manually and publish them by hand once. I don't have the secret stuff to publish. I am pretty sure the order is

oq3_lexer, oq3_parser, oq3_syntax, oq3_source_file, oq3_semantics.

@jakelishman
Copy link
Member

John: yeah, at least for the Qiskit package I have with a direct dependency on oq3_semantics, the (reduced) cargo tree looks like this:

jake@ninetales$ cargo tree  | rg 'oq3_'
├── oq3_semantics v0.0.1 (https://github.com/Qiskit/openqasm3_parser.git?rev=88211a6#88211a67)
│   ├── oq3_source_file v0.0.1 (https://github.com/Qiskit/openqasm3_parser.git?rev=88211a6#88211a67)
│   │   ├── oq3_syntax v0.0.1 (https://github.com/Qiskit/openqasm3_parser.git?rev=88211a6#88211a67)
│   │   │   ├── oq3_lexer v0.0.1 (https://github.com/Qiskit/openqasm3_parser.git?rev=88211a6#88211a67)
│   │   │   ├── oq3_parser v0.0.1 (https://github.com/Qiskit/openqasm3_parser.git?rev=88211a6#88211a67)
│   │   │   │   ├── oq3_lexer v0.0.1 (https://github.com/Qiskit/openqasm3_parser.git?rev=88211a6#88211a67) (*)
│   ├── oq3_syntax v0.0.1 (https://github.com/Qiskit/openqasm3_parser.git?rev=88211a6#88211a67) (*)

which agrees with the order you said.

@jlapeyre jlapeyre merged commit 84236e7 into Qiskit:main Jan 21, 2024
7 checks passed
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 this pull request may close these issues.

None yet

3 participants