-
Notifications
You must be signed in to change notification settings - Fork 711
Implement --benchmark-options for v2-bench #6224
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
I have confirmed that I can run
Well, mostly. What is up with this part?
The package whose benchmark I'm running ( |
That looks like #5119. |
While it's possible that it's "just" #5119, it's worth noting that with this patch, the extraneous warning now appears any time you run |
Actually, I retract my claim in #6224 (comment) — I don't think my patch caused this warning. That's because I get the same warning if I run |
Thanks a lot for implementing this @RyanGlScott! The new options work very nicely for me! 👍 |
On certain Travis configurations (e.g.,
The thing is, I'm not actually sure how to reproduce this failure locally. I'm testing with |
With @hvr's help (thanks!), I believe I figured out how to fix the issue in #6224 (comment). Let's see if Travis agrees with me. |
The AppVeyor build is segfaulting:
As far as I can tell, this is #6219. This failure also happens on |
I think I've resolved all outstanding issues. Ready for a proper review now! |
Would someone knowledgeable in the area be willing to give this a review? |
A gentle ping on this. |
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.
Ok, that looks good now.
Just one last order of buissness: adding a test. I checked and --test-option(s)
has a test in cabal-testsuite/PackageTests/TestOptions/setup.test.hs
so you should be able to use that as a template.
I saw that too, and in fact, there's already a corresponding test called |
With enough persistence, I was able to cobble together a test case that exercises the |
Great! Just one more thing, can you make the test ensure that multiple Since we're relying a lot on genericlly derived Monoid instances and it's not clear at all from the code alone that these flags append I think we should test for that. If you feel like it a test for v2-test for symmetry would also be good, in a separate commit please. |
I've modified the |
cabal-testsuite/PackageTests/NewBuild/CmdTest/OptionsFlag/cabal.test.hs
Outdated
Show resolved
Hide resolved
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.
Thanks for doing the v2-test test too :)
I think we're ready for merging as soon as you address the two remaining minor comments.
This implements lots of plumbing to allow the `--benchmark-option(s)` flags to be used with `v2-bench`, analgous to `v2-test`'s `--test-option(s)` flag. Fixes #6209.
Hm, the CI at https://travis-ci.org/haskell-pushbot/cabal-binaries/builds/596608715 seems to have timed out. Moreover, I don't have the ability to restart it (likely due to lack of permissions for https://github.com/haskell-pushbot/cabal-binaries). What should I do? |
Upon further investigation, it looks like the OS X jobs are known to time out frequently. Moreover, an upstream commit has explicitly allowed these jobs to fail (see #6278). In light of this, I'm going to just land this as-is. |
Yeah don't worry about the OSX jobs too much they're really flaky, but I see you merged it already :) |
This implements lots of plumbing to allow the
--benchmark-option(s)
flags to be used withv2-bench
, analgous tov2-test
's--test-option(s)
flag.Fixes #6209.
Please include the following checklist in your PR:
[ci skip]
is used to avoid triggering the build bots.Please also shortly describe how you tested your change. Bonus points for added tests!