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

Added scheduled release workflow #461

Merged
merged 1 commit into from May 29, 2020
Merged

Added scheduled release workflow #461

merged 1 commit into from May 29, 2020

Conversation

pksunkara
Copy link
Contributor

@pksunkara pksunkara commented May 20, 2020

As per the discussion on Zulip, here's the action that schedules and releases chalk every Sunday.

I used cargo-workspaces for this which means if there are no changes in a week, a version will not be released. It also intelligently releases only the changes instead of everything.

You need to add 2 secrets in github for this to work:

  • PERSONAL_ACCESS_TOKEN: Github personal access token that has rights to push to the master branch of this repo
  • CARGO_REGISTRY_TOKEN: Crates.io token of the user who has access to publishing all the chalk crates

You can see an example release commit here

cc @jackh726 @flodiebold @nikomatsakis

I saw your message about asking me to also integrate bors-ng. While I did do that for https://github.com/clap-rs/clap, I am not sure if I can do that here without having access to settings. I could just create/edit the related config for it but it would be hard for me to debug.

EDIT: I realized that I would need to update Cargo.lock as well. Can someone confirm? While I would like to just run cargo build to update the Cargo.lock, but I think it would also update other deps, right? Is there an option to update only the local crates? I am not sure how to proceed on this.

Does running cargo update -p {crate_name} for each crate in workspace fix it?

@AzureMarker
Copy link
Member

Can this tool also bump the versions to a dev version after publishing?

@AzureMarker
Copy link
Member

For updating Cargo.lock, you could do cargo update -p chalk chalk-solve chalk-engine .... cargo update -p chalk* might work if the shell expands the glob.

@pksunkara
Copy link
Contributor Author

Can this tool also bump the versions to a dev version after publishing?

I didn't think that is necessary. Is there any reason for doing this?

@AzureMarker
Copy link
Member

I'm not sure if it's necessary, but I've observed it happening here.

@pksunkara
Copy link
Contributor Author

The tool now updates the Cargo.lock. sample commit

As you can see in that commit, only the changed packages got released. The next run would update to 0.13.0 if changes are added to a package that wasn't updated in the current run

@jackh726
Copy link
Contributor

This is really nice :) So, I know it doesn't update the version to add a -dev tag. I really like doing this, since it allows anyone to keep track of whether or not the code on master is released. (I've so often looked at a codebase and had to track down if the code I'm seeing is the a released version). If we can make it do this, I think I'm completely on board. (I might be able to submit a PR to cargo-workspaces if that what it takes).

Other than that, does it do everything on the checklist? If so, then I could maybe be persuaded that not adding a -dev tag is okay.

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
chalk-solve/Cargo.toml Show resolved Hide resolved
@pksunkara
Copy link
Contributor Author

does it do everything on the checklist?

It does everything except for the following things:

  • add a -dev suffix
  • editing RELEASES.md

I really like doing this, since it allows anyone to keep track of whether or not the code on master is released. I've so often looked at a codebase and had to track down if the code I'm seeing is the a released version

Just doing git log should show you all the info you need since every release is tagged. Also, doing this suffix not only complicates the release process but also complicates the usage of these libraries using git in others by a little bit.

The work on cargo-workspaces needed for this is not much and I can do it, but i just don't understand the reason for it.

@jackh726
Copy link
Contributor

So, the reasoning for a RELEASES.md is for users to have a general overview of the changes that went into a release. But we discussed it in a meeting, and I don't think anyone was opposed to not having one. And I don't think anyone wanted e.g. just a paste in of git log. So I'm mostly okay with not keeping it updated.

How does adding the suffix complicate the usage of the crates? If someone is using the git repo as a dependency, then they don't require a version. If they publish, then there has to be a crates.io release of the version they need. Which is not the same as the git master.

@AzureMarker
Copy link
Member

Why not update RELEASES.md in the PRs? That's a common and easy way to keep it up to date with the code changes.

@pksunkara
Copy link
Contributor Author

If they publish, then there has to be a crates.io release of the version they need. Which is not the same as the git master.

Yeah, but imagine if the releases are non-breaking. For example, I would use crate A in B like the following:

A = { git = "git://github.com/rust/A", branch = "master", version = "1.2" }

I would want to develop my crate against the git master but since A follows semver I can safely use the version keyword which is used when I am publishing B. But if A starts adding -dev tags`, most of the time it's not an issue, but when the suffix is added to an incompatible version, that's when things start to get complicated.

But I understand that this basically comes down to the general preference of the maintainer and if you want it I will add it.

@jackh726
Copy link
Contributor

Well, I don't think cargo looks at version when pulling a git dependency. So it doesn't matter what the version is on master. (-dev or not). Also, while the releases follow semver, the master branch does not. So using master as a dependency but falling back to a version could theoretically break. (In fact, the only time we are guaranteed to not break by semver is if master would only be bumping to a patch version. E.g. if we are relying on new features in master)

If it's not a lot of work, it would be awesome if you could add it.

Thanks :)

@pksunkara
Copy link
Contributor Author

pksunkara commented May 22, 2020

Did it. You can see the same run here without the publishing ofcourse.

This is the release commit generated by that run.
This is the release commit that adds the dev prerelease tag.

This PR is now ready I think

Copy link
Contributor

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

We probably don't want the ".0" in the -dev.0. Other than that LGTM

@nikomatsakis what do you think of this? I think the only user-visible change here is we no longer will be updating the RELEASES.md. We also need to set up the tokens.

@AzureMarker
Copy link
Member

Why not update RELEASES.md in the PRs? That's a common and easy way to keep it up to date with the code changes.

#461 (comment)
Not sure if this was seen, but I'm confused why we would stop updating RELEASES.md. For example, in the const-generics PR we could add a line saying "added const generics support" under the next minor version, simple as that.

@jackh726
Copy link
Contributor

@Mcat12 I did miss your comment, sorry.

We did discuss this in the meeting too. I don't think having people update the RELEASES.md in commits is a bad idea, but if this workflow doesn't update the version there too, then it's not quite as useful. The biggest argument against this, I think, is it just creates extra things to keep track of for a (potentially) small gain.

@pksunkara
Copy link
Contributor Author

It's not a big deal to update it. Just a sed command should take care of it. But we haven't been using it thus I didn't do it.

@jackh726
Copy link
Contributor

Ooh then that would be ideal. (I do think it is a nice thing to have. Even if it's a little bit more work)

@AzureMarker
Copy link
Member

AzureMarker commented May 22, 2020

I have previously looked into cargo-release for my own crates, and it supports running sed-like commands during the update: https://github.com/sunng87/cargo-release/blob/master/docs/faq.md#maintaining-changelog

Maybe something similar can be added with cargo-workspaces.

@pksunkara
Copy link
Contributor Author

pksunkara commented May 22, 2020

Just a sed command should take care of it

I was wrong.

If we want to use this generally, we wouldn't know the version until cargo-workspaces would be run. But once it's run, we can't update the RELEASES.md. (Well, we could, but the updated RELEASES.md will not be present in the tagged commit). To correctly handle this, I would have to implement search and replace functionality in the tool like how cargo-release did which unfortunately is more work than I expected.

@AzureMarker
Copy link
Member

We could stay one version ahead in the changelog. Then we just add a section for the next version when bumping post-release. Ex: after releasing 0.10, add a 0.11 section to the changelog.

@pksunkara
Copy link
Contributor Author

So, I had to add some manual steps with git commit & git tag, etc.. but I did it with sed now. You can see it here

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

I have no strong opinion about this setup but I'd like to request a review from @pietroalbini or someone else from @rust-lang/infra

@pksunkara
Copy link
Contributor Author

Master is failing due to recent merge of @Mcat12's PR

@AzureMarker
Copy link
Member

It looks like the PR wasn't fully up to date with master and the panic tests which were in master reference the old chalk-rust-ir crate. I'm on mobile for the next few hours, otherwise I would make a PR to fix the panic test imports.

@pksunkara
Copy link
Contributor Author

We should definitely have bors

@jackh726
Copy link
Contributor

I fixed master and restarted the checks

@pksunkara
Copy link
Contributor Author

@pietroalbini @nikomatsakis Only the cargo token is left to be added and this thing can start publishing.

@nikomatsakis
Copy link
Contributor

OK. I'm debating if I should make a "dummy user" to nikomatsakis-chalk to just host the publishing credentials here.

@nikomatsakis
Copy link
Contributor

Since crates.io tokens are kind of coarse-grained :)

Copy link
Contributor

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

We probably don't want the ".0" in the -dev.0.

Not sure if this was seen/addressed. If so, then LGTM. Otherwise, this should be changed.

@pksunkara
Copy link
Contributor Author

I have been trying to find some standardized info on the pre-release tags but am unable to. AFAIK, the pre-release tags should always be tagged as word.num where num starts with 0 and all of this can be followed by a build tag.

This has been my experience and the other release tools which support prereleases. But as I said, I can't find this written down anywhere.

@jackh726
Copy link
Contributor

@pksunkara See the "Backus–Naur Form Grammar for Valid SemVer Versions" section at https://semver.org/

Specifically, the <version core> "-" <pre-release>

@pksunkara
Copy link
Contributor Author

Let me rephrase, yes it is possible to write anything for the pre-release, but there is a convention majority people follow and I was describing it. I built the cargo-workspaces prerelease functionality around this convention. I don't think I can easily fix this.

@jackh726
Copy link
Contributor

I have always seen it as just -dev, or if it's alpha/beta maybe alpha.0. But we're only doing -dev. Why does cargo-workspaces auto-append a .0 to a prerelease tag? Does that preclude have, say, multiple alphas?

@pksunkara
Copy link
Contributor Author

Yeah, the assumption behind it was that pre releases will always be numbered since they can be multiple with the same tag.

@AzureMarker
Copy link
Member

The difference here seems to be that we never intend on publishing the -dev versions.

@pietroalbini
Copy link
Member

OK. I'm debating if I should make a "dummy user" to nikomatsakis-chalk to just host the publishing credentials here.

That'd be the best way, even though I'd lean torwards a bot account managed by the infra team (I can set it up).

@nikomatsakis
Copy link
Contributor

@pietroalbini a bot sounds better, yes

@nikomatsakis
Copy link
Contributor

fyi, @pietroalbini was looking into creating an infra bot to host the token, but he ran into some problem that he's trying to resolve.

@pietroalbini
Copy link
Member

Added the CARGO_REGISTRY_TOKEN secret, but it does not work yet. I'll post here once it's ready to be used.

@pksunkara
Copy link
Contributor Author

What's the issue? Just curious.

@pietroalbini
Copy link
Member

The issue we had before was that the bot account we created got flagged, so we talked with GitHub to get it fixed. Now I need to coordinate with Niko to add ownership of the crates to the bot.

@jackh726 jackh726 merged commit ffa2e51 into rust-lang:master May 29, 2020
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

5 participants