feat: pkg.pr.new on main #354
Conversation
|
@Aslemammad FYI the CI is failing |
|
Yes I will tackle it soon. |
| pkg.scripts.selftestscript = | ||
| "[ -d ../../vite/packages/vite/dist ] || (echo 'vite build failed' && exit 1)" | ||
| if (options.release?.startsWith('https://pkg.pr.new/vite@')) { | ||
| pkg.scripts.selftestscript = |
There was a problem hiding this comment.
@sapphi-red would love to hear your thoughts on this now that the ci is fixed by this!
utils.ts
Outdated
| cd(vitePath) | ||
| export async function getPermanentRef(repo: string, ref: string) { | ||
| try { | ||
| const ref = await $`git log -1 --pretty=format:%H` | ||
| return ref | ||
| const res = await fetch( | ||
| `https://api.github.com/repos/${repo}/branches/${ref}`, | ||
| ) | ||
| const { | ||
| commit: { sha }, | ||
| } = (await res.json()) as { commit: { sha: string } } | ||
| return sha |
There was a problem hiding this comment.
I don't think this is correct. What we want for the usage in discord-webhook.ts is the commit hash that the ecosystem-ci ran on. This is getting the latest commit hash when the ci finished (i.e. a different hash will be output when a new commit was pushed while the CI is running). (It is fine for the case of ecosystem-ci.ts as it's called before running.)
There was a problem hiding this comment.
this should always use the local checkout if it exists and as mentioned by sapphi we can't have ambiguity introduced by new commits. So in case where no local checkout exists, you would have to obtain the ref in a deterministic way. Currently not sure what that is other than asking for it to be passed in (ie never just a branch name but always branch+revision)
ecosystem-ci.ts
Outdated
| if ( | ||
| options.branch === 'main' && | ||
| options.repo === 'vitejs/vite' && | ||
| !options.commit | ||
| ) { | ||
| const sha = await getPermanentRef(options.repo, options.branch) | ||
| if (sha) { | ||
| options.commit = sha | ||
| } | ||
| } | ||
| if (options.commit) { | ||
| const url = `https://pkg.pr.new/vite@${options.commit}` | ||
| //eslint-disable-next-line n/no-unsupported-features/node-builtins | ||
| const { status } = await fetch(url) | ||
| if (status === 200) { | ||
| options.release = url |
There was a problem hiding this comment.
By this code, we will sometimes get a prebuilt package and sometimes don't, depending on when the last commit was pushed. I think we should avoid that if possible so that there aren't differences among the runs.
There was a problem hiding this comment.
i see, can you show me an instance where we give the prebuilt package? i'd like to see how can i manipulate the code to perform well on those edge-cases.
i wonder if putting if (options.commit && !options.release) would resolve that edge case or no? would love to hear your thoughts.
There was a problem hiding this comment.
The example case of using a prebuilt package is:
- I push a commit in the main branch at 10:30.
- The CI in Vite repo build the package and uploads it at 10:32.
- The ecosystem-ci triggers at 10:35.
- The ecosystem-ci uses the prebuilt package.
The example case of not using a prebuilt package is:
- I push a commit in the main branch at 10:30.
- The ecosystem-ci triggers at 10:31.
- The ecosystem-ci does not use a prebuilt package.
- The CI in Vite repo build the package and uploads it at 10:32.
i wonder if putting
if (options.commit && !options.release)would resolve that edge case or no?
I think that doesn't resolve the case.
|
will resolve the comments asap! |
Co-authored-by: 翠 / green <[email protected]>
Co-authored-by: 翠 / green <[email protected]>
ecosystem-ci.ts
Outdated
| let attempts = 0 | ||
| do { | ||
| const { status } = await fetch(url) | ||
| if (status !== 200) { |
There was a problem hiding this comment.
why is this !== instead of the previous === ?
There was a problem hiding this comment.
hey this is for debugging and i need to make few changes.
ecosystem-ci.ts
Outdated
|
|
||
| console.log(`continuous release available on ${url}`) | ||
| } | ||
| const maxAttempts = 60 // 5 minutes |
There was a problem hiding this comment.
currently vite-ecosystem-ci takes less than one minute to build vite from source. I don't think polling 5 minutes to wait for it to maybe appear on pr.new is the right way to go here.
Either make sure the ecosystem-ci run is only triggered after pr.new is available or skip ahead and build it here.
There was a problem hiding this comment.
I think it's important to make it consistent across commits, so I'd prefer not to skip ahead. How about throwing an error if it exceeds 1 minutes? The built package should be available at that time.
There was a problem hiding this comment.
i don't think ecosystem-ci should be reliant on pr.new availability so fallback is needed.
I'd prefer the wait/poll was implemented at the trigger side, not here
There was a problem hiding this comment.
I'd prefer the wait/poll was implemented at the trigger side, not here
The trigger is here in this repo.
vite-ecosystem-ci/.github/workflows/ecosystem-ci.yml
Lines 15 to 17 in 66423f9
There was a problem hiding this comment.
it should still go into that job then but not convinced we need this. what's the advantage of pr.new on scheduled runs?
There was a problem hiding this comment.
hey! sorry for the long wait, it's been a bit hard on my side.
it has the same benefits that it has on triggered runs from PRs! it'd avoid the building part on each scheduled run since there's a high chance that commit was already built.
And if it was not built, we wait a bit until it'd be.
But anyway, let me know, I'm ok with making either polling or falling back work.
There was a problem hiding this comment.
building vite here takes 14 seconds (thanks rolldown!)
is it really going to be faster to check pr.new? Esp. the polling variant feels like overkill. If its there and usable immediately, ok. but else just do it here. scheduled runs (and PR runs too) are unlikely to hit the same commit hash twice, so unless it is cached already there is no benefit for ecosystem-ci
There was a problem hiding this comment.
I'd like to keep the behavior consistent across the runs, otherwise there'll be problems that are difficult to investigate like vitejs/vite#20080 (comment).
If the polling is overkill, I think we should always built it here.
Workflow runs for the main branch do not use the pkg.pr.new build, meanwhile the main branch of the vite repo is the place where each commit gets its own pkg.pr.new build which feels a bit wasted.
So this will try to look for pkg.pr.new builds for the main branch.