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

Use default resolver for test 4101-dependency-tree #4407

Merged
merged 1 commit into from
Nov 20, 2018
Merged

Conversation

qrilka
Copy link
Contributor

@qrilka qrilka commented Nov 20, 2018

Please include the following checklist in your PR:

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

That's a fix for a new test. Fix in #4402 also resolved a similar problem (apart from fixing GHCi behavior) - its stack was using LTS-12 while integration tests use install-ghc:false -

writeFile (newStackRoot </> "config.yaml") "system-ghc: true\ninstall-ghc: false\n"

I guess it's worth to check new integration tests when they appear whether they use the default resolver. This way we could prevent this kind of problem.

@dbaynard
Copy link
Contributor

Yes - perhaps calling stack in tests should use defaultResolverArgs by default.

I could do with some help with the test for 4181 - it's tangentially related to this. Would you have a chance to look at it, @qrilka?

@qrilka
Copy link
Contributor Author

qrilka commented Nov 20, 2018

@dbaynard what do you want me too look at @dbaynard ? Do you mean the code in #4390?

@dbaynard
Copy link
Contributor

No, it's #4181 (ae16993). I accidentally set it up with an invalid resolver, which means it can pass the test. It perhaps ought to use defaultResolverArgs.

That test should ensure that stack clean does not attempt to download ghc, and fail if it does. I'd thought I could hide ghc by editing the path, though I didn't have much joy. Another pair of eyes would be helpful.

Meanwhile, I'll review this 👍

@qrilka
Copy link
Contributor Author

qrilka commented Nov 20, 2018

@dbaynard probably I needed to be a bit more explicit what was the problem this PR tries to solve:
LTS-12 needs GHC 8.4.3 and the default LTS-11.22 uses 8.2.2. At the same time we use install-ghc: false in integration tests.
The commit you reference uses GHC-8.2.2 so it shouldn't be a problem at all.

There's a test 4085-insufficient-error-message which uses --resolver nightly-2018-06-05 --install-ghc but that one should be OK as it does it in a Docker image.
Please let me know if you need any more information.

@qrilka
Copy link
Contributor Author

qrilka commented Nov 20, 2018

Ouch, I only now noticed that it's 8.22 instead of 8.2.2, will take another look

@qrilka
Copy link
Contributor Author

qrilka commented Nov 20, 2018

@dbaynard what about using something explicitly not available like ghc-8.0.2 and add a meaninful comment about that value? The current comment # Update the resolver as necessary in that stack.yaml is way too misleading.
Otherwise it looks to be a reasonable test - no suggestions how it could be made in a better way.

@dbaynard
Copy link
Contributor

what about using something explicitly not available like ghc-8.0.2 and add a meaningful comment about that value?

That sounds good — I'll do that.

Incidentally, this test (4101) will see some more changes going forward as there is more work done for ls dependencies --tree.

@dbaynard dbaynard merged commit 0b68efc into master Nov 20, 2018
@qrilka qrilka deleted the 4101-test-fix branch December 28, 2018 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants