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: remove appveyor specific code + fix publish benmark results race #3071

Merged
merged 1 commit into from Oct 6, 2019

Conversation

@chrmoritz
Copy link
Contributor

commented Oct 5, 2019

This PR:

  • removes some no longer needed appveyor specific code paths
  • significantly reduces the likelihood for a race condition when 2 concurrent CI jobs try to publish benchmark results, fixes #3062
  • makes benchmark result publishing slightly more robust by not silently swallowing gh-pages branch checkout failures and starting from new with an empty history (and only run the gh-pages checkout on CI)
@chrmoritz chrmoritz force-pushed the chrmoritz:ci branch from fea228c to adbe5dd Oct 5, 2019
Copy link
Collaborator

left a comment

Is this missing a change to .github/workflow? I don’t see how gh-pages gets checked out.

@chrmoritz

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2019

@ry gh-pages gets checked out still in benchmark.py by calling import_data_from_gh_pages(). However it's called later at the end now, after actual running the benchmarks, but still before combining the new data with the old data, because it's needed at least than.

In theory we could grab the old date using raw.githubusercontent.com there and clone the gh-pages branch in the Post Benchmarks workflow. But this would only postpone the cloning by a few seconds (if at all) and doesn't significantly reduce the likelihood of such an race condition.

"git", "clone", "--depth", "1", "-b", "gh-pages",
"https://github.com/denoland/deno.git", "gh-pages"
])
return read_json(gh_pages_data_file)

This comment has been minimized.

Copy link
@ry

ry Oct 6, 2019

Collaborator

This logic is too complex. I would prefer we move the git clone to .github/workflows/build.yml ... I realize that will take some refactoring of how the data gets merged - and not a trivial fix.
I'd happily land the other clean ups - how about reverting this part for now?

This comment has been minimized.

Copy link
@chrmoritz

chrmoritz Oct 6, 2019

Author Contributor

So what you want is to remove all the code handling the historic data from gh-pages from benchmark.py and make it to just write the current result to a file?

Then we could create another small script which runs only on the CI which merges the new data into the existing data of an gh-pages checkout. Do I understand this correctly?

I drop the benchmark.pyfor now, might look into it again tomorrow.

This comment has been minimized.

Copy link
@ry

ry Oct 6, 2019

Collaborator

Yes that’s what I was thinking... now that you write it out I worry it might be even more complex... but somehow I think having benchmarks.py clone is not properly separating concerns.

@chrmoritz chrmoritz force-pushed the chrmoritz:ci branch from adbe5dd to f9c057a Oct 6, 2019
@ry
ry approved these changes Oct 6, 2019
Copy link
Collaborator

left a comment

LGTM - thank you

@ry ry merged commit 33e3ff5 into denoland:master Oct 6, 2019
6 checks passed
6 checks passed
test macOS-10.14
Details
test windows-2016
Details
test ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.