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

ci(actions): trying to create auto upgrade PR for swc_* #9

Closed
wants to merge 1 commit into from

Conversation

kwonoj
Copy link
Collaborator

@kwonoj kwonoj commented Dec 3, 2022

This PR trying to experiment automated action to create a PR to upgrade swc_* when a new version is released. swc_core is changing rapidly, and there are several pieces we need to align when we want to upgrade dependencies altogether so I hope this could reduce some of manual steps.

It is an action runs periodically (2 times a daily) which runs cargo upgrade, then create a PR if there's an update to be applied. If there is an open PR with a specific branch name, it won't create new but trying to update PR with new commit when there's an update.

To create fully working pull request, action requires personal access token unfortunately. I choose name GHA_UPGRADE_TOKEN for those, having one under repository secrets will enable action to perform:

image

We can use default GITHUB_TOKEN still, but it'll create PR with limited acesss with github-actions bot will not trigger any action to verify build is passing or not. (ref: peter-evans/create-pull-request#48)

Copy link
Contributor

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @kwonoj!

I'm generally in favor of auto updating 👍
Though that has been a point of discussion more broadly across the JS side of mdx/remark/unified, where auto updaters can create a lot of PR noise.

If an auto-updater could make sense here, and not generate too much noise, I'm in favor.
I'd also note, renovate bot has a fairly comprehensive and configurable solution https://docs.renovatebot.com/rust
Which has been used in a few places in the mdx js implementation, configured to mostly do major version update PRs.
And that may be easier to maintain than a bespoke GHA solution.

@kwonoj
Copy link
Collaborator Author

kwonoj commented Dec 3, 2022

As long as I can get an fast followup for the swc_core release, I'm totally fine to close pr.

Currently each time I am creating a manual pr, but in most cases it is straightforward verion update doesn't have to be manual and as long as we can automate it it should be good.

@kwonoj
Copy link
Collaborator Author

kwonoj commented Dec 3, 2022

Also thanks for the heads up, I didn't know renovate has rust support.

@wooorm
Copy link
Owner

wooorm commented Dec 4, 2022

Currently each time I am creating a manual pr, but in most cases it is straightforward verion update doesn't have to be manual and as long as we can automate it it should be good

I currently see two PRs: one with changes, one without:

So I wonder how much this will help: if it’s 50%/50%, it still requires frequent work.
Especially because I guess that SWC is not on a stable semver major version because it wants to introduce a lot of breaking changes: if it was at ^1.0.0, we could let semver do this job.

Other than updating here, we’d also need to cut releases.
Perhaps I can also give you access to do so. That way, whenever SWC bumps, it can be bumped here and in Next.js/etc as well, on your schedule.

@kwonoj
Copy link
Collaborator Author

kwonoj commented Dec 4, 2022

While we hope to do so, swc_core is currently not close to follow strict semver and we have very frequent cases to update / align our (next-swc/turbopack)'s transitive dependencies to have explicit version bump to swc_core to solve certain problems. It is notable even if it's not a breaking changes, version bump is needed somtimes - for example, last bump was to resolve circular dependencies issue in swc_core itself only was able to be solved by upgrading all of swc_core in next-swc's dependency tree at once. So while there aren't quantified numbers when we need version upgrade or not, I'd say we'll need more frequent than current for various reasons.

I'm not trying to land this PR as an answer as said above. As long as there's some automated process 1. upgrade swc_core automatically, if it's not a breaking changes 2. notify someone for 1 if it's breaking 3. automatically publish when there is a bump, that'll solve the problem I want to resolve.

@wooorm
Copy link
Owner

wooorm commented Dec 5, 2022

So while there aren't quantified numbers when we need version upgrade or not, I'd say we'll need more frequent than current for various reasons.

It sounds like Next.js has a complex release line that requires manual work to align everything at once, and cut releases right after that. I don’t think any automation here can solve that problem.
Particularly because right now the data of 2 PRs is 50% that can be automatic, 50% that require manual intervention.

As long as there's some automated process 1. upgrade swc_core automatically, if it's not a breaking changes 2. notify someone for 1 if it's breaking 3. automatically publish when there is a bump, that'll solve the problem I want to resolve.

1) and 2) can be done with Renovate. @ChristianMurphy Perhaps you could set this up here, following the current/unified style, as you have the most experience here?
I’m open to trying this out for a month or more or so, and we can re-evaluate later.

3) is more complex. I would like this project to follow semver, and soonish cut a 1.0.0, and while I don’t consider rethrown errors as breaking that, I can foresee updating SWC to introduce breaking changes here due to its current volatility. I guess it boils down to that I don’t trust computers. After a couple months of testing renovate, we can perhaps re-evaluate?

But we could solve 3) by giving you access to crates?

@kwonoj
Copy link
Collaborator Author

kwonoj commented Dec 5, 2022

It sounds like Next.js has a complex release line that requires manual work to align everything at once, and cut releases right after that. I don’t think any automation here can solve that problem.

Yes, totally agreed. This is something we want to make improvements & fixes over time, but honestly we do not have clear answers for this yet. The effort we're trying to achieve is somewhat trial & error to see what's working and what's not.

The ideal world is something like DI then caller / upstream have right control for the dependencies with strict semver-following, as we all know that's something we don't have it, unfortunately.

For the 3, since this is a public-facing package I get the concerns of breaking changes. I do not have strong opinions for this yet, since my problem is more facing to aligning swc_core only.

Maybe, it'd be better we (next.js or other upstream) trying to make an attempt to fork this logics and freely bump up swc_core whenever we want would be better approach? then we can upstream to this repo occasionally and mdxjs-rs itself can maintain its semver strictness on its release plan. Though I'm not sure if this is really good idea to fork just to handle one of the dependencies version.

@wooorm
Copy link
Owner

wooorm commented Dec 6, 2022

Though I'm not sure if this is really good idea to fork just to handle one of the dependencies version.

So sounds like overrides in package.json for npm, right? https://docs.npmjs.com/cli/v8/configuring-npm/package-json#overrides

Maybe https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html?
Then in Next, you can pin one specific version of SWC, that will be used by this crate and by other next crates?

See also the “Specific versions” section on https://stackoverflow.com/questions/27770031/how-do-i-pin-indirect-dependencies-of-a-crate

This was referenced Jan 23, 2023
@ChristianMurphy
Copy link
Contributor

ChristianMurphy commented Jan 25, 2023

  1. and 2) can be done with Renovate. @ChristianMurphy Perhaps you could set this up here, following the current/unified style, as you have the most experience here?

Opened #12 with a potential setup for renovate

wooorm pushed a commit to wooorm/markdown-rs that referenced this pull request Jan 31, 2023
Related-to: wooorm/mdxjs-rs#9.

Reviewed-by: Titus Wormer <[email protected]>
@wooorm wooorm closed this in 5d2e914 Jan 31, 2023
@wooorm
Copy link
Owner

wooorm commented Jan 31, 2023

@kwonoj Let’s try with Renovate for a bit, see how that goes, and evaluate later?

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.

3 participants