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

build: add github action for publishing crates #1012

Closed
wants to merge 5 commits into from

Conversation

manmartgarc
Copy link

@manmartgarc manmartgarc commented Sep 6, 2023

Summary

First attempt at automating the publishing of libcst and libcst_derive to the crates.io registry on release and push to the main branch. Closes #967.

Notes

  • The --allow-dirty flag was needed during testing since for some reason the files in the workflow are not committed to git and cargo publish complains about this.
  • Not sure if we should keep the libcst_derive dependency of libcst as a local path dependency or if it should be changed to point to the crates.io registry.

Test Plan

The repository owner should set up a new secret with the crates.io token, and name it CRATES_IO_TOKEN so that the workflow can use the token when publishing to the registry.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 6, 2023
Copy link
Member

@zsol zsol left a comment

Choose a reason for hiding this comment

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

This will publish to the version hardcoded in Cargo.toml, right? One of the nice things in the pypi workflow is that the version is derived from git, so we:

  1. Don't have to make sure the version number in the two Cargo.toml files match the git tag and pyproject.toml
  2. Can publish intermediate versions to a test registry; this is great for testing the release pipeline itself

Is there anything we can do to have these properties on the rust side?

command: publish
args: >
--manifest-path ./native/libcst_derive/Cargo.toml
--allow-dirty
Copy link
Member

Choose a reason for hiding this comment

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

why --allow-dirty?

Copy link
Author

Choose a reason for hiding this comment

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

I was testing locally with nektos/act and was getting an issue where cargo publish wouldn't allow files that weren't committed, even though the files were actually committed. Not sure if this would be the case on GitHub

@manmartgarc
Copy link
Author

This will publish to the version hardcoded in Cargo.toml, right? One of the nice things in the pypi workflow is that the version is derived from git, so we:

  1. Don't have to make sure the version number in the two Cargo.toml files match the git tag and pyproject.toml
  2. Can publish intermediate versions to a test registry; this is great for testing the release pipeline itself

Is there anything we can do to have these properties on the rust side?

I am assuming we should be able to derive the version from git, will check more on this. Regarding the second question there is a --dry-run flag that can be passed to cargo publish which will do everything up to publishing the crates on a registry.

@manmartgarc
Copy link
Author

This will publish to the version hardcoded in Cargo.toml, right? One of the nice things in the pypi workflow is that the version is derived from git, so we:

  1. Don't have to make sure the version number in the two Cargo.toml files match the git tag and pyproject.toml
  2. Can publish intermediate versions to a test registry; this is great for testing the release pipeline itself

Is there anything we can do to have these properties on the rust side?

It looks like someone is doing this by replacing the version in the Cargo.toml file. Is this something you'd be open to?

@manmartgarc manmartgarc marked this pull request as draft September 21, 2023 07:06
@manmartgarc manmartgarc marked this pull request as ready for review September 23, 2023 05:43
@manmartgarc
Copy link
Author

f23af55 adds:

  • the functionality to do a --dry-run release on push and do the full release on release.
  • Hackily replace the versions in the Cargo.toml files by the one in the current git tag using sed.

Not sure if this is good enough, but I wasn't able to find a homegrown way of using the tag in cargo publish. Another thing to note is that libcst is published using the local version of libcst_derive. I am not sure if we should also publish libcst_derive but publishing the latter might be avoided if only libcst should be on the registry.

@codecov-commenter
Copy link

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (37277e5) 91.04% compared to head (f23af55) 91.04%.
Report is 5 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1012   +/-   ##
=======================================
  Coverage   91.04%   91.04%           
=======================================
  Files         255      255           
  Lines       26366    26366           
=======================================
  Hits        24004    24004           
  Misses       2362     2362           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@manmartgarc
Copy link
Author

Huh. Why is there a typing error if no source changes were made?

@zsol
Copy link
Member

zsol commented Oct 1, 2023 via email

@zsol
Copy link
Member

zsol commented Oct 2, 2023

FTR, I fixed this in #1032

@manmartgarc
Copy link
Author

FTR, I fixed this in #1032

I'll be able to merge this into the PR later today 👍

@manmartgarc
Copy link
Author

I had made a mess of the branch with a git pull --rebase but it looks like now the git history looks better. Not sure how this change is affecting the actions yet. Could you approve the workflow to see what happens?

@manmartgarc
Copy link
Author

Bumping this. Are there any blockers currently?

@manmartgarc
Copy link
Author

@zsol checking in on this. I see that the right versions are already on crates.io: https://crates.io/search?q=libcst. Is there anything else to do here?

@manmartgarc
Copy link
Author

@zsol should I close this PR? I see v1.0.1 on crates.io, thinking this might be all set up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publishing the parser to crates.io
4 participants