-
Notifications
You must be signed in to change notification settings - Fork 841
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
Make stack honor --skip flag #4449
Conversation
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.
Hi @vanceism7, thanks for the PR!
The code change looks fine — and I see you've got a detailed integration test.
For the integration test, as I understand it, you have two packages:
this
that
that
should build.
this
should not.
The problem is, because the library for this
does not build, there is no point testing the executable and the test.
It may be worth replacing the two libraries with one, but passing flags, e.g.
flags:
library:
description: The library compiles
default: false
manual: true
executable:
description: The executable compiles
default: false
manual: true
tests:
description: The tests compile
default: false
manual: true
Then pick modules based on whether those flags are set.
So, with --flag this:library
the library will be buildable, so a build without --skip this
should succeed. Etc.
Beyond that, it would be good to ensure that internal libraries are skipped (or otherwise) correctly — that's probably more important.
Sounds good?
-- The lib, exe, and test components of "this" package will fail if built. | ||
-- But they should be skipped | ||
stack ["build", "--skip", "this-exe", "--skip", "this"] | ||
stack ["test", "--skip", "this-test", "--skip", "this-exe", "--skip", "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.
e.g. (Note: I haven't changed the first line but it's the only way to do a suggested addition)
stack ["test", "--skip", "this-test", "--skip", "this-exe", "--skip", "this"] | |
stack ["test", "--skip", "this-test", "--skip", "this-exe", "--skip", "this"] | |
stackErr ["build"] | |
stackErr ["test"] |
It may be worth adding more granularity, e.g.
stackErr ["build", "--skip", "this"]
|
||
-- The lib, exe, and test components of "this" package will fail if built. | ||
-- But they should be skipped | ||
stack ["build", "--skip", "this-exe", "--skip", "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.
You should note that this builds that
successfully, but not this
.
Also what should happen for the following?
stack ["build", "--skip", "this-eve", "--skip", "this", "this"]
Stack shouldn't build anything! This should be tested, too.
test/integration/tests/3827-skip-libs-bug/files/this/app/Main.hs
Outdated
Show resolved
Hide resolved
test/integration/tests/3827-skip-libs-bug/files/this/src/Lib.hs
Outdated
Show resolved
Hide resolved
test/integration/tests/3827-skip-libs-bug/files/this/test/Spec.hs
Outdated
Show resolved
Hide resolved
Makes clear these files shouldn't compile Co-Authored-By: vanceism7 <[email protected]>
I see what you mean about the failing build for the this lib. I'm not sure how the whole flags aspect of builds works. It looks like they are flags belonging to cabal build system itself, is there some sort of docs I can find for those? I tried looking but all I really found was cabal-install stuff. I could also alternatively just do a check to see if the |
Yes, they are Cabal flags. I'll take a look at this. |
Thanks for your patience, @vanceism7. I've done the basic setup; I'm just running into other bugs while trying to choose the actual test commands! Perhaps ominously, |
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've made some changes to the test lines. They don't work for me. I think there's an issue with buildable
in cabal files, but even your test lines didn't work as I expected.
(Oh, and I've spotted some mistakes in my last commit which I'll fix here.)
@vanceism7 Just pinging on open PRs, are you still planning on continuing with this? |
Hey sorry guys, I've meant to respond but have been a little busy. I believe my original tests were working but they've been changed pretty drastically I think, and I don't know that my tests were really the correct tests to use in the first place. I don't think I'll really have time to finish up the testing issues for this though. Sorry about that! |
Understood @vanceism7, no problem. I'll try to get this over the finish line, thanks for your work on this PR 👍 |
Alright, after digging into this a bit more, it's going to make a lot more sense to try and tackle this after we have component based builds in place, so I'm going to delay this for now. CC @qrilka |
The ultimate solution for this problem will likely be quite different, so I'm going to close this out for now. |
This should fix the issue of libs not being skipped when specified in the skip flags #3827.
I added skipping for both internal libs and foreign libs, but not sure if this is totally correct or needed. Just let me know if I need to change anything, the fix was pretty minimal anyhow