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

ci: Fix --index-state for hackage roundtrip tests #10285

Merged
merged 2 commits into from
Aug 27, 2024
Merged

Conversation

mpickering
Copy link
Collaborator

As a principle, tests which are required for CI to pass should be
reproducible and not depending on external resources changes or being
modified. The hackage tests currently violate this by depending on the
latest index state from hackage. This is problematic because until the
test is fixed all merges into master are blocked. Even though the
patches in question have nothing to do with the test.

It would be more suitable for a nightly job to run on the latest index
and for normal CI to run with a fixed index which is updated
periodically in a controlled manner.

Fixes #10284

We need to fix the index-state we test against so a new bad cabal file
doesn't take down the CI for everyone.

Towards #10284
As a principle, tests which are required for CI to pass should be
reproducible and not depending on external resources changes or being
modified. The hackage tests currently violate this by depending on the
latest index state from hackage. This is problematic because until the
test is fixed all merges into master are blocked. Even though the
patches in question have nothing to do with the test.

It would be more suitable for a nightly job to run on the latest index
and for normal CI to run with a fixed index which is updated
periodically in a controlled manner.

Fixes #10284
Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

I fully support the idea. But I'm also worried about the proliferation of pinned index states in this repo. It'd almost be better to use grep in validate.sh to extract the index state from one of the project files instead.

Copy link
Collaborator

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

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

Looks good. Perhaps we could skip the merge delay for this one as it is blocking every other PR.

And probably it would be worth creating an issue pointing to where the error in the parsing comes from now that you already investigated that a bit.

@jasagredo jasagredo added the merge me Tell Mergify Bot to merge label Aug 27, 2024
@fgaz
Copy link
Member

fgaz commented Aug 27, 2024

Merging now to unbreak CI

@fgaz fgaz added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 27, 2024
@mergify mergify bot merged commit 1ff41e8 into master Aug 27, 2024
52 checks passed
@mergify mergify bot deleted the wip/hackage-tests-fix branch August 27, 2024 15:24
@geekosaur
Copy link
Collaborator

@mergify backport 3.12

Copy link
Contributor

mergify bot commented Aug 30, 2024

backport 3.12

✅ Backports have been created

mergify bot added a commit that referenced this pull request Sep 13, 2024
…10305)

* hackage-tests: Add --index-state argument to fix the cabal files

We need to fix the index-state we test against so a new bad cabal file
doesn't take down the CI for everyone.

Towards #10284

(cherry picked from commit 8e4d167)

* ci: Fix --index-state for hackage roundtrip tests

As a principle, tests which are required for CI to pass should be
reproducible and not depending on external resources changes or being
modified. The hackage tests currently violate this by depending on the
latest index state from hackage. This is problematic because until the
test is fixed all merges into master are blocked. Even though the
patches in question have nothing to do with the test.

It would be more suitable for a nightly job to run on the latest index
and for normal CI to run with a fixed index which is updated
periodically in a controlled manner.

Fixes #10284

(cherry picked from commit 31507b1)

* Re-enable Windows CI

(cherry picked from commit 4aade2d)

* CI: skip cli-suite on Windows due to #9571 (#10257)

(cherry picked from commit 30d2a38)

---------

Co-authored-by: Matthew Pickering <[email protected]>
Co-authored-by: Javier Sagredo <[email protected]>
Co-authored-by: Artem Pelenitsyn <[email protected]>
@geekosaur
Copy link
Collaborator

Could someone who's analyzed this please create an issue as mentioned above? I'm trying to look into why this is failing, and all I've got so far is a missing test-suite and that the PackageDescription has been (possibly deliberately) ellipsized; and TBHJ I don't have a whole lot of an idea what I'm looking at, aside from the obvious that it's failing to parse io-classes-1.6.0.0's cabal file.

@mpickering
Copy link
Collaborator Author

@geekosaur I think you need #10287 and the related issue #10283

@geekosaur
Copy link
Collaborator

Ah, so it's already fixed, but the PR is pending on 2 open discussions and a merge conflict (validate.sh is a stub for `cabal-validate; the corresponding change, which I made for my own local test, is at https://github.com/haskell/cabal/blob/master/cabal-validate/src/Main.hs#L276-L285). (Linking this comment in the other PR.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hackage roundtrip tests should use a fixed index state
5 participants