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

Relax extension .c requirement for c-sources #9285

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

hamishmack
Copy link
Collaborator

#9200 started filtering .cc files from c-sources and does not always issue a warning (there has to be at least one .c file present before it warns you it is ignoring the others).

Some packages in hackage (double-conversion for instance) rely on the ability to include C++ source files in c-sources. This is not ideal, but we should probably continue to support these packages.

This change will relax the filtering, so that only .h files are automatically excluded (with a warning). It will also warn if other non .c files are present (suggesting the cxx-sources since C++ sources are the most likely to be used).

The bug that prevented warnings being displayed when no .c files were present is fixed.

haskell#9200 started filtering `.cc` files from `c-sources` and does not always issue a warning (there has to be at least one `.c` file present before it warns you it is ignoring the others).

Some packages in hackage (`double-conversion` for instance) rely on the ability to include `C++` source files in `c-sources`.  This is not ideal, but we should probably continue to support these packages.

This change will relax the filtering, so that only `.h` files are automatically excluded (with a warning).  It will also warn if other non `.c` files are present (suggesting the `cxx-sources` since C++ sources are the most likely to be used).

The bug that prevented warnings being displayed when no `.c` files were present is fixed.
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.

Thank you.

@Kleidukos Kleidukos added the merge me Tell Mergify Bot to merge label Sep 25, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 27, 2023
@hamishmack
Copy link
Collaborator Author

Is something wrong with mergify?

@hamishmack
Copy link
Collaborator Author

Does it only automatically merge into master?

@ulysses4ever
Copy link
Collaborator

@mergify refresh

@mergify
Copy link
Contributor

mergify bot commented Oct 3, 2023

refresh

✅ Pull request refreshed

@ulysses4ever
Copy link
Collaborator

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Oct 3, 2023

rebase

✅ Nothing to do for rebase action

@ulysses4ever
Copy link
Collaborator

Mergify only merges backports to non-master branches (and those should be clearly marked: in the description and by the label), that's why this PR is not handled by it.
Why does this PR target the 3.10 branch if it's not a backport? This is rarely needed, and if it is, it has to be merged manually.

@hamishmack
Copy link
Collaborator Author

It fixes an issue with #9200 and that PR did not seem to be in master as far as I could tell.

@ulysses4ever
Copy link
Collaborator

@hamishmack makes sense now, thank you.

@Kleidukos are we still accepting PRs for the 3.10 branch? If so, this one will have to be merged manually (Mergify only serves backports to non-master branches).

@Kleidukos more importantly: do you know why #9200 doesn't seem to have a version of it on master? It'd be embarassing to publish a fix in 3.10.2 (3.10.3?) to only break this again in 3.12...

@Kleidukos
Copy link
Member

Kleidukos commented Oct 4, 2023

@ulysses4ever yes this one is going in

@Kleidukos more importantly: do you know why #9200 doesn't seem to have a version of it on master

I forgot.

@Kleidukos Kleidukos merged commit e2cc066 into haskell:3.10 Oct 4, 2023
37 checks passed
@Mikolaj
Copy link
Member

Mikolaj commented Oct 4, 2023

@Kleidukos more importantly: do you know why #9200 doesn't seem to have a version of it on master? It'd be embarassing to publish a fix in 3.10.2 (3.10.3?) to only break this again in 3.12...

I think, #9200 is a quick patch to make cabal releasable, but we haven't had time to discuss at length and agree on a full long-term fix. So perhaps I closed #9190 prematurely, after all. However, given that nobody objected to my closing the issue, perhaps this two-PR fix is good enough? In that case we could just forwardport it to master. Let me ask on the closed issue again.

@hamishmack
Copy link
Collaborator Author

I think ghc 9.8.1 was released without this fix.

@Kleidukos
Copy link
Member

@hamishmack GHC is released with lib:Cabal as a boot library because it really is needed for GHC. People who will use cabal-install will use the one we distribute, with the backport.

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.

4 participants