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 type field optional for tests and benchmarks #8115

Merged
merged 4 commits into from
May 1, 2022

Conversation

cydparser
Copy link
Collaborator

@cydparser cydparser commented Apr 27, 2022

Resolves #7459

I manually tested the change using cabal check against the various combinations of fields and stanzas.


Please include the following checklist in your PR:

@Mikolaj
Copy link
Member

Mikolaj commented Apr 27, 2022

Well done! A quick git grep exitcode-stdio shows there's a lot of tests with this field. I wonder, perhaps you could pick a couple of such tests, maybe best if they have some very similar counterparts in the test suite, and remove the field in them (but not in the counterparts, to keep testing the parsing of the field in the same context) and tweak the expected outputs accordingly? We may want to avoid tests that specifically test the testing harness so as not to derail their main purpose.

@cydparser
Copy link
Collaborator Author

Thanks @Mikolaj. I've done as you suggested and removed type from some tests. I've also fixed the failing #5055 tests, but wonder whether they should be removed instead.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2022

fixed the failing #5055 tests, but wonder whether they should be removed instead.

As far as I understand, these tests really do test the changed functionality, so I'd keep them. I can't say I understand the functionality well, though (#5055), but perhaps we shouldn't worry too much.

removed type from some tests

Perfect. CI broke randomly, so let me restart.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Apr 29, 2022

rebase

☑️ Nothing to do

  • -closed [:pushpin: rebase requirement]
  • #commits-behind>0 [:pushpin: rebase requirement]

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2022

The same CI failure (Windows: Access violation in generated code when reading 0xffffffffffffffff) and the same cancellation of jobs without an obvious reason. Github status is green, but I suspect github outage, regardless. No other cabal CI jobs running right now to compare to, so I've rebased PR #8038 to compare to. Let's see how it goes.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2022

Copying from #hackage:

is it possible this is a genuine linker/libs/whatever error and the other jobs got cancelled, because GHA is smart and noticed the required job that depends on all of them won't be green anway, due to this single failure?

(the jobs that got cancelled are not required, but they are dependencies of a required job AFAICT)

the disaster just got completely reproduced in another PR I just rebased

given that it's Windows and the relatively old GHC 8.4.4 I vote to disable this CI job with a "fluke" comment and worry only if it repeats for any other job

Once we disable the offending CI job, let's rebase to repeat the tests.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Apr 29, 2022

rebase

✅ Branch has been successfully rebased

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2022

Splendid. Now CI green. :)

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jneira jneira 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, that is a sensible default, thanks!

@jneira jneira merged commit 3a0989d into haskell:master May 1, 2022
@fgaz
Copy link
Member

fgaz commented May 10, 2022

This changes the cabal spec, and needs a changelog entry and version guards, or we'll have to revert before release.

@Mikolaj
Copy link
Member

Mikolaj commented May 10, 2022

Details for the 3.8 TODO:

The changelog in question is https://github.com/haskell/cabal/blob/master/doc/file-format-changelog.rst

The code change may be something like #8134 and #8135 (when done.

cydparser added a commit to cydparser/cabal that referenced this pull request May 11, 2022
+ Update file-format-changelog.rst
cydparser added a commit to cydparser/cabal that referenced this pull request May 11, 2022
+ Update file-format-changelog.rst
cydparser added a commit to cydparser/cabal that referenced this pull request May 11, 2022
+ Update file-format-changelog.rst
andreabedini pushed a commit to cydparser/cabal that referenced this pull request May 16, 2022
+ Update file-format-changelog.rst
mergify bot added a commit that referenced this pull request May 16, 2022
Require `type` field when spec < 3.8 (#8115)
@cydparser cydparser deleted the optional-type branch May 17, 2022 04:03
Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Changes requested: Clarification concerning cabal-version needed.

doc/cabal-package.rst Show resolved Hide resolved
doc/cabal-package.rst Show resolved Hide resolved
doc/cabal-package.rst Show resolved Hide resolved
doc/cabal-package.rst Show resolved Hide resolved
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.

Feature request: assume type: exitcode-stdio-1.0 by default
5 participants