Skip to content
This repository has been archived by the owner on Oct 29, 2022. It is now read-only.

Fix #90 (re-enable autobenchmarking) #91

Closed
wants to merge 3 commits into from

Conversation

JasonGross
Copy link
Member

Apparently variables left empty in the configuration don't get set at
all, rather than being set to the empty string. So we disable the
variable checks for variables allowed to be empty.

Fixes #90

Apparently variables left empty in the configuration don't get set at
all, rather than being set to the empty string.  So we disable the
variable checks for variables allowed to be empty.

Fixes coq#90
@ejgallego ejgallego self-assigned this Jun 29, 2020
@JasonGross
Copy link
Member Author

Why is bignums failing to install?

INFO: failed to install coq-bignums

@ejgallego
Copy link
Member

Why is bignums failing to install?

Likely the hash for the CI has to be bumped, that's a weakness of the current CI setup [PRs should update this hash]

@JasonGross
Copy link
Member Author

Okay, bumped. Couldn't we use master and master^ for the commit "hashes", and thereby be more robust in the future?

@ejgallego
Copy link
Member

Couldn't we use master and master^ for the commit "hashes", and thereby be more robust in the future?

Sure, if that works that would be great; I'm unsure what git can care from our variables tho.

@ejgallego
Copy link
Member

I'm gonna push a bit more robust CI and then merge.

@JasonGross
Copy link
Member Author

@ejgallego Any update on this?

@ejgallego
Copy link
Member

I will take care of this ASAP, sorry for the delay.

@ejgallego
Copy link
Member

With Jenkins offline I'm unsure what to do here :S

@JasonGross
Copy link
Member Author

@maximedenes @ejgallego I'd be inclined to merge this, and propagate the changes to #12581, so the code does not bitrot. Thoughts?

@ejgallego
Copy link
Member

@JasonGross I'm OK with any choice here, I'm afraid that I'm not up to date w.r.t. bench plans anymore so you'll have to sync with others.

@maximedenes
Copy link
Member

@JasonGross now that we moved the bench infrastructure to Gitlab CI, is this still relevant?

Sorry for the confusion, we had to move quickly because we were notified of more security issues with Jenkins by the Inria IT services.

@JasonGross
Copy link
Member Author

A lot of it is still relevant, yes. I think that it would still be nice for coqbot to automatically post the results of benchmarking to the PR, and it would be nice to be able to trigger benchmarking just by adding the needs:benchmarking label to the script. Should I prepare a version of this against coq/coq, now that the script lives there?

@maximedenes
Copy link
Member

Should I prepare a version of this against coq/coq, now that the script lives there?

It would be nice yes, as the two features you mention certainly sound useful.

@JasonGross
Copy link
Member Author

Actually, it looks like someone (@ppedrot?) already integrated all of this into the main Coq repo's bench script, so there's nothing to be done unless you want to sync the script in this repo with the one in coq/coq (in which case you should probably just merge this PR).

as the two features you mention certainly sound useful.

I think basically all that's left for these is the work on the coqbot side, as the script should already be sending post requests in any bench job where the coq_pr_number variable is set. (Well, I guess we also have to update the script to point at the right locations. I'll go prepare a PR to do that.)

@SkySkimmer
Copy link
Contributor

trigger benchmarking just by adding the needs:benchmarking label

That's too agressive, the label can mean "after necessary fixups are done"

@JasonGross
Copy link
Member Author

Perhaps it can trigger when someone comments @coqbot: benchmark?

@ejgallego ejgallego removed their assignment Sep 22, 2020
@ejgallego
Copy link
Member

Not relevant after the migration IIANM.

@ejgallego ejgallego closed this Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regular builds fail: coq_pr_number / comment_id must be set
4 participants