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

Unbreak Criterion's HTML reports #764

Merged
merged 1 commit into from
Jan 26, 2021

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Jan 25, 2021

Our benchmarking harness has decided that it's time for some INNOVATION and for starters released a version which has disabled generation of HTML reports by default, changed their location from current directory to the proper shared target directory, and strongly suggests migrating away from running benchmarks with cargo bench in favor of their own cargo criterion.

At least they have a changelog. It does not yet contain an entry for 0.3.4 released about 15 hours ago, but that's something.

Well, okay, that's a fine rabbit to chase later, but for now let's at least unbreak our build by adapting to the new reality.

Checklist

  • Change is covered by automated tests
  • Benchmark results are attached (should be)

Our benchmarking harness has decided that it's time for some INNOVATION
and for starters released a version which has disabled generation of
HTML reports by default, changed their location from current directory
to the proper shared target directory, and strongly suggests migrating
away from running benchmarks with "cargo bench" in favor of their own
"cargo criterion".

Well, okay, that's a fine rabbit to chase later, but for now let's at
least unbreak our build by adapting to the new reality.
@ilammy ilammy added core Themis Core written in C, its packages infrastructure Automated building and packaging tests Themis test suite labels Jan 25, 2021
@ilammy ilammy requested review from vixentael, shadinua, iamnotacake and a team January 25, 2021 11:34
Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

world is always changing ¯_(ツ)_/¯

Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

looks like has no backdoors

Copy link
Contributor

@iamnotacake iamnotacake left a comment

Choose a reason for hiding this comment

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

So, for now we use cargo_bench_support feature to be able to run old command cargo bench -- 'Secure Cell .* master key/4 KB' and have tests passing? Well, okay, good to see stuff fixed

@ilammy
Copy link
Collaborator Author

ilammy commented Jan 26, 2021

@Lagovas,

looks like has no backdoors

Oh, you've read the entire code of criterion 0.3.4 and its 18 direct dependencies? That's amazing! 🤣

@ilammy
Copy link
Collaborator Author

ilammy commented Jan 26, 2021

@iamnotacake,

So, for now we use cargo_bench_support feature to be able to run old command

Yeah, that's the plan. It is supposed to be working with 0.3.x branch, but I guess they might break it in 0.4. Cargo will prevent that upgrade for happening accidentally, but I guess it's a good idea to migrate to cargo criterion soon™ (which in practice probably means “when the build breaks next time, but this time irreversibly”).

@ilammy ilammy merged commit d230f57 into cossacklabs:master Jan 26, 2021
@ilammy ilammy deleted the bring-back-html-reports branch January 26, 2021 12:37
@vixentael
Copy link
Contributor

vixentael commented Feb 1, 2021

Could this PR be a reason why tests are failing during nightly jobs? @ilammy

Looks like it can't find a folder to create report.

https://app.circleci.com/pipelines/github/cossacklabs/themis/995/workflows/007bce5b-d029-4f13-b0d8-d9ce9228e5e3/jobs/17700
Screenshot 2021-02-01 at 02 16 42

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 1, 2021

Mmm... Maybe. Likely.

CircleCI should be active only on stable. And on stable this patch is not present, hence it's still broken. I guess it could be cherry-picked there, and adapted to CircleCI, just to unbreak it.

@ilammy ilammy mentioned this pull request Feb 1, 2021
1 task
@ilammy
Copy link
Collaborator Author

ilammy commented Feb 1, 2021

Also, now that you have brought this to my attention...

GitHub Actions are not executed on schedule for the stable branch. No nightly builds there.

There are workflows there, sure, with all this

  schedule:
    - cron: '0 6 * * *' # every day at 6:00 UTC

but it's not getting executed on stable with this configuration, only master.

That's because:

on.schedule

Scheduled workflows run on the latest commit on the default or base branch.

https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#onschedule

Quickly skimming through the internet reveals that we should not need this feature, we’re weird for wanting it, and we should probably start by purchasing Azure subscription before we can demand that to be fixed.

I wonder what may be a workaround. Ideally, one that does not involve duplicating all the workflows with different branch checked out. And one that still runs tests for changes in PRs.

Ideas? @iamnotacake?

I admit that I was too hasty to drop CircleCI orz

ilammy added a commit that referenced this pull request Feb 2, 2021
Our benchmarking harness has decided that it's time for some INNOVATION
and for starters released a version which has disabled generation of
HTML reports by default, changed their location from current directory
to the proper shared target directory, and strongly suggests migrating
away from running benchmarks with "cargo bench" in favor of their own
"cargo criterion".

Well, okay, that's a fine rabbit to chase later, but for now let's at
least unbreak our build by adapting to the new reality.

(cherry picked from commit d230f57)

Also added the same path update for CircleCI which is still running for
the stable branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Themis Core written in C, its packages infrastructure Automated building and packaging tests Themis test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants