-
Notifications
You must be signed in to change notification settings - Fork 49.7k
Update results.json from master before the build #11882
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
Conversation
| # Update the local size measurements to the master version | ||
| # so that the size diff printed at the end of the build is | ||
| # accurate. | ||
| curl -o scripts/rollup/results.json http://react.zpao.com/builds/master/latest/results.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hesitate to bring it up, but should we move this to reactjs.org?
Not that, like, @zpao will hold our build ransom or anything :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah maybe :-) I don't care strongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, no worries. We need to figure out static hosting for the fixtures too. Maybe we can figure this out when/if we deal with that.
|
Verified it shows 0% as expected. |
|
Won't this compare PRs that may not be based off master against master? Since we don't merge/rebase afaik. You want to compare against the parent commit… |
|
Yea, so I'm not sure about that. What is the thing we want to show? In my experience branching off to explore size savings I still end up wanting to compare to master since that's where the branch will ultimately go. |
|
I'm thinking of the (most common) case where PRs are independent. A big size increase lands in master. You send a one-line PR based on the commit before that. You get blamed for the size increase. Certainly in cases where we expect two PRs to interact it's sort of relevant to compare against master, but in that case we should just rebase the PR manually anyway. |
|
Hmm. Looks like there's no easy way to get I'll chat to @zpao and see what we can do. |
|
Another option could be to use the CircleCI artifacts API but it looks like that also doesn't have a trivial commit -> artifact API. |
|
Chatted with @gaearon - The timestamp is based off the commit, so it's fairly easy to rebuild (though you do have to shell out). Here's the upload step: https://github.com/facebook/react/blob/master/scripts/circleci/upload_build.sh#L15, just need to add But I'll can also make it so that we have symlinks directly into the right folder. That's a totally reasonable thing to want, I think I've wanted it before too… |
This should ensure that CI always displays correct build file difference. A prerequisite for #11865.
We might actually want to delete that file from the repo and always download it during the local build. Then we can delete it from the repo, always have accurate stats on local builds, and solve merge conflicts. But then we need to make sure we bail out gracefully if network fails.