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

Make stack honor --skip flag #4449

Closed
wants to merge 10 commits into from

Conversation

vanceism7
Copy link
Contributor

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

Copy link
Contributor

@dbaynard dbaynard left a 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?

test/integration/tests/3827-skip-libs-bug/files/stack.yaml Outdated Show resolved Hide resolved
test/integration/tests/3827-skip-libs-bug/Main.hs Outdated Show resolved Hide resolved
-- 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"]
Copy link
Contributor

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)

Suggested change
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"]
Copy link
Contributor

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.

Makes clear these files shouldn't compile

Co-Authored-By: vanceism7 <[email protected]>
@vanceism7
Copy link
Contributor Author

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 .stack-work directory exists instead of making a failing build?

@dbaynard
Copy link
Contributor

Yes, they are Cabal flags. I'll take a look at this.

@mihaimaruseac mihaimaruseac changed the title Fix skip #3827 Make stack honor --skip flag Jan 22, 2019
@dbaynard
Copy link
Contributor

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, stack build --skip this:lib this:lib doesn't seem to work, even on your version, so it's a little hard for me to judge. Would you please confirm which test cases you can get to work (and which you can't)?

Copy link
Contributor

@dbaynard dbaynard left a 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.)

test/integration/tests/3827-skip-libs-bug/Main.hs Outdated Show resolved Hide resolved
test/integration/tests/3827-skip-libs-bug/Main.hs Outdated Show resolved Hide resolved
test/integration/tests/3827-skip-libs-bug/Main.hs Outdated Show resolved Hide resolved
@snoyberg
Copy link
Contributor

@vanceism7 Just pinging on open PRs, are you still planning on continuing with this?

@vanceism7
Copy link
Contributor Author

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!

@snoyberg
Copy link
Contributor

Understood @vanceism7, no problem. I'll try to get this over the finish line, thanks for your work on this PR 👍

@snoyberg
Copy link
Contributor

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

@snoyberg
Copy link
Contributor

The ultimate solution for this problem will likely be quite different, so I'm going to close this out for now.

@snoyberg snoyberg closed this Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants