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

Downgrade NoLibraryFound from an error to a warning #9926

Merged
merged 1 commit into from
May 6, 2024

Conversation

TeofilC
Copy link
Collaborator

@TeofilC TeofilC commented Apr 24, 2024

This makes Setup copy/install succeed if there's
nothing to do because the package doesn't contain
a library or executable.

This allows downstream users of Cabal to avoid having to add workarounds for this edge case.

Resolves #6750

Let me know what you think. If this sounds OK, I'll add a test case, which test suite would be best?


  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

@TeofilC TeofilC force-pushed the wip/succeed-on-nothing-copy branch 2 times, most recently from 7980205 to 92ab485 Compare April 24, 2024 11:06
@Mikolaj Mikolaj added attention: needs-review re: error-message Concerning error messages delivered to the user re: warnings Concerning warnings printed by cabal labels Apr 24, 2024
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, though I'm not familiar with these code paths.

Re tests, the default seems to be cabal-testsuite.

@TeofilC TeofilC force-pushed the wip/succeed-on-nothing-copy branch from 92ab485 to 94cee1f Compare April 29, 2024 11:52
@TeofilC
Copy link
Collaborator Author

TeofilC commented Apr 29, 2024

I've added a test case of a package that is just a test suite. In fact this is one of the options offered by the cabal init setup wizard

@TeofilC TeofilC force-pushed the wip/succeed-on-nothing-copy branch from 94cee1f to 5fb3e36 Compare April 29, 2024 15:12
Copy link
Collaborator

@alt-romes alt-romes left a comment

Choose a reason for hiding this comment

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

Thanks. I don't see a need to emit an error here so this seems like an improvement.

@TeofilC TeofilC force-pushed the wip/succeed-on-nothing-copy branch from 5fb3e36 to 6d9ea26 Compare April 30, 2024 10:36
@Mikolaj
Copy link
Member

Mikolaj commented May 1, 2024

@TeofilC: could you kindly set a merge label, confirming you finished tweaking the PR, as per https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#github-pull-request-conventions? Thank you.

@TeofilC TeofilC added the merge me Tell Mergify Bot to merge label May 1, 2024
@TeofilC
Copy link
Collaborator Author

TeofilC commented May 1, 2024

Sounds good. Thanks!

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label May 3, 2024
@Mikolaj Mikolaj force-pushed the wip/succeed-on-nothing-copy branch from 6d9ea26 to c12f295 Compare May 3, 2024 13:04
@Mikolaj
Copy link
Member

Mikolaj commented May 6, 2024

A very strange CI error out of a sudden:

<<< /Users/runner/work/cabal/cabal/dist-newstyle-validate-ghc-9.4.8/build/x86_64-osx/ghc-9.4.8/Cabal-tests-3/t/parser-tests/build/parser-tests/parser-tests -j2 --hide-successes (1/37 sec) 

-j2 --hide-successes
validate.sh: line 94: -j2: command not found

in https://github.com/haskell/cabal/actions/runs/8939442661/job/24555485188?pr=9926

Let me restart it.

This makes Setup copy/install succeed if there's
nothing to do because the package doesn't contain
a library or executable.

This allows downstream users of Cabal to avoid having to
add workarounds for this edge case.

Resolves #6750
@Mikolaj Mikolaj force-pushed the wip/succeed-on-nothing-copy branch from c12f295 to 312a412 Compare May 6, 2024 12:41
@mergify mergify bot merged commit fd8020f into master May 6, 2024
51 checks passed
@mergify mergify bot deleted the wip/succeed-on-nothing-copy branch May 6, 2024 15:31
@ulysses4ever
Copy link
Collaborator

@mergify backport 3.12

Copy link
Contributor

mergify bot commented Jun 6, 2024

backport 3.12

✅ Backports have been created

mergify bot added a commit that referenced this pull request Jun 7, 2024
…10076)

* Downgrade NoLibraryFound from an error to a warning

This makes Setup copy/install succeed if there's
nothing to do because the package doesn't contain
a library or executable.

This allows downstream users of Cabal to avoid having to
add workarounds for this edge case.

Resolves #6750

(cherry picked from commit 312a412)

# Conflicts:
#	Cabal/src/Distribution/Simple/Install.hs

* fixup! fix conflicts

---------

Co-authored-by: Teo Camarasu <[email protected]>
Co-authored-by: Artem Pelenitsyn <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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 re: error-message Concerning error messages delivered to the user re: warnings Concerning warnings printed by cabal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SETUP copy fails if only test or benchmark
5 participants