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

Enables running test-loop against different lts versions #53

Merged

Conversation

qxjit
Copy link
Member

@qxjit qxjit commented Nov 27, 2018

In doing so this also allows for more travis builds on different lts
versions, which I also enabled.

In doing so this also allows for more travis builds on different lts
versions, which I also enabled.
@qxjit qxjit requested a review from telser November 27, 2018 20:23
Copy link
Contributor

@telser telser left a comment

Choose a reason for hiding this comment

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

Always great to see compatibility with more compiler versions!

There is a tested-with field for package.yaml that let's one specify what compilers and versions were tested against. Maybe we want to use that since we are testing with so many stackage.org lts versions now?

Something else that I've seen with other projects is keeping a default stack.yaml with the newest version supported. Not sure if that's something we want to do here. Personally don't have a strong opinion one way or another for that, but not sure if you've seen that pattern.


# the following dependencies are for
# the test suite
- tasty-discover-4.2.1
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to use a lower version of tasty-discover in lts-6.35 given that we are only testing with tasty-discover-2.0.3 on lts-8.24. Seems reasonable that if someone is all the way back on this old of an lts that they'll also be on an older version of tasty.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's reasonable, and I'll certainly go ahead and make the change. That said, I don't imagine that the hypothetical person in question would be using our stack-lts-6.35.yml in the first place even if they are compiling our test suite.

compiler: ": #stack nightly osx"
os: osx

allow_failures:
- env: BUILD=cabal GHCVER=head CABALVER=head HAPPYVER=1.19.5 ALEXVER=3.1.7
- env: BUILD=stack ARGS="--resolver lts-12"
- env: BUILD=stack ARGS="--resolver nightly"
- env: BUILD=stack ARGS="--stack-yaml stack-lts-10.10.yml --resolver nightly"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the line above this not need to change to specify the yml file as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, the line above should simply be removed. We're not currently running that build, and once the conduit work is done it won't be an allowed failure either. Good catch.

@@ -0,0 +1,6 @@
resolver: lts-10.10
Copy link
Contributor

Choose a reason for hiding this comment

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

I recall a conversation around the compiler flags used to build by default given hackage recommendations. After getting some warnings from Orville trying to use -Weverything perhaps we could turn on more strict warnings for these so we see those in CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not 100% sure what you're getting at, but if you're saying you want to simply print warnings during CI I honestly doubt we'd every go and look at them very much. If you're suggesting we make them errors, I'm concerned that trying to make everything to compile on so many different versions without any warnings on any of them might be a lot of effort for not much gain.

I'm not against trying if we think there's value in it, but I'd like to understand what course of action you're specifically suggesting before getting too far down the path.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just trying to get us some way to have visibility into more warnings, especially ones that end up being shown to users of Orville. And since we don't want Weverything to be on for all compiles for package repositories, this seemed like an opportunity to be able to get those warnings in a consistent way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't buy that having CI spit out warnings is going to cause is to fix them. If our goal is to never have a user see a warning while Orville compiles, we would need to commit to using -Werror and going through whatever gymnastics are necessary to fix all warnings across all compiler versions. At the very least I'm not willing to do that right now.

@qxjit
Copy link
Member Author

qxjit commented Nov 28, 2018

@telser Re: tested-with, thanks for pointing that out. I'll figure out what it means to have that in here.

Re: having a default stack.yml, I did consider that. I rejected for the moment in favor of:

  • consistency in names of the various stack-*.yml files we have now
  • controlling the "default" version in the docker-compose.yml

I think I'd be wiling to change this if we had an outside contributor who felt strongly about it for some reason, but absent that I prefer this approach.

This is in line with the version being used with lts-8.24 and
also has the advantage of not required Glob as a extra-dep
@telser
Copy link
Contributor

telser commented Nov 28, 2018

@qxjit
Re: tested-with, I'm really curious to know what the usage of it is, but I have seen it in many package descriptions as of late.

Re: the default stack.yml, the only thing I don't like about the approach you outlined is that it ties the default to docker. When I do development, it isn't with docker so myself and any others using a containment system other than docker miss out on that.

@qxjit
Copy link
Member Author

qxjit commented Nov 28, 2018

@telser I've been investigating tested-with today. It appears to be consumed by this script for generating .travis.yml files and nothing else ever. This is informed by this github issue comment regarding tested-with. The field isn't even displayed on the hackage page for projects that have the field present.

@qxjit
Copy link
Member Author

qxjit commented Nov 28, 2018

@telser For the time being, I would encourage you to use the docker setup. If it's causing pain, I would like to understand why and attempt to fix it. I very much prefer the explicit versioning and consistency to the "one of these stack files is special" setup.

This documents the GHC versions that we have travis builds for. We
should keep it up to date as we add more.
This documents the GHC versions that we have travis builds for. We
should keep it up to date as we add more.
@qxjit qxjit merged commit a443b11 into 0.9_development Nov 28, 2018
@qxjit qxjit deleted the qxjit/ch13089/support-dev-with-different-lts-versions branch November 28, 2018 20:11
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