Skip to content

Implement extra-files #603

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

Merged
merged 1 commit into from
Mar 14, 2025
Merged

Implement extra-files #603

merged 1 commit into from
Mar 14, 2025

Conversation

mpilgrem
Copy link
Collaborator

@mpilgrem mpilgrem commented Feb 23, 2025

cabal-version: 3.14 of the Cabal package description format specification has introduced new field extra-files - like extra-source-files but without the semantics that the files listed are tracked by build tools.

@mpilgrem mpilgrem requested a review from sol February 23, 2025 19:29
Copy link
Owner

@sol sol left a comment

Choose a reason for hiding this comment

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

Hey, thanks for looking into this.

Personally I am not convinced that extra-files offers enough value over extra-source-files and extra-doc-files to be worthwhile.

That said, this is a minimal change, so in this specific case I am willing to accept this patch for parity with Cabal.

Note that parity with Cabal is not a stated design principle of Hpack, so this will always be a case by case decision.

README.md Outdated
@@ -143,6 +143,7 @@ library:
| `build-type` | · | `Simple`, or `Custom` if `custom-setup` exists | Must be `Simple`, `Configure`, `Make`, or `Custom` | | |
| `extra-source-files` | · | | Accepts [glob patterns](#file-globbing) | | |
| `extra-doc-files` | · | | Accepts [glob patterns](#file-globbing) | | `0.21.2` |
| `extra-files` | · | | Accepts [glob patterns](#file-globbing) | | `0.39.0` |
Copy link
Owner

Choose a reason for hiding this comment

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

As this is a very non-invasive change, do you think we should release as 0.38.1?

NB: Adding a new field to Hpack.Config.Package I don't consider a "breaking" change:

  • Hpack.Config is semi-internal
  • A user should use Hpack.Config.package to construct a Package anyway, and if they did that, their code will continue to work just fine.
  • In general, for records with a large number of fields, I as a user should always be prepared that new fields are added.

Copy link
Owner

Choose a reason for hiding this comment

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

Please also bump the package version accordingly, so that a release to Hackage is triggered

@sol sol changed the title Fix #602 Implement extra-files Implement extra-files Mar 12, 2025
@sol
Copy link
Owner

sol commented Mar 12, 2025

@mpilgrem I updated the issue description, can you please update the commit message accordingly?

@mpilgrem
Copy link
Collaborator Author

This is 'early', in that only GHC >= 9.12 requires Cabal >= 3.14 and we are unlikely to understand user use of extra-files until library authors start dropping support for GHC 9.10. However, Cabal-based build tools will treat extra-files differently (changes not tracked, not copied to html directory) from extra-source-files and extra-doc-files.

@sol
Copy link
Owner

sol commented Mar 12, 2025

Cabal-based build tools will treat extra-files differently (changes not tracked, ...) from extra-source-files

My question is more along the lines of "When does this actually matter?".

I think in the worst case you just rebuild too often. Now if there is some extra file that changes all the time, then I would see a potential issue that extra-files could address. It's just that I can't imagine a use case where this would matter.

Copy link
Owner

@sol sol left a comment

Choose a reason for hiding this comment

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

  • apply my two diff suggestions
  • bump version to 0.38.1
  • add change log entry

Then this is ready to go 👍

@@ -638,6 +638,20 @@ spec = around_ (inTempDirectoryNamed "my-package") $ do
- "*.markdown"
|] `shouldWarn` ["Specified pattern \"*.markdown\" for extra-doc-files does not match any files"]

describe "extra-files" $ do
it "accepts a list of files" $ do
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
it "accepts a list of files" $ do
it "accepts extra-files" $ do

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We currently have:

extra-doc-files
  accepts a list of files

and

extra-files
  accepts extra-files

seemed a little 'circular' as an output - but if you are sure that is what you want, I'll put it through.

Copy link
Owner

Choose a reason for hiding this comment

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

The priorities are:

  1. It needs to be clear what the purpose of a specific test case is ("What are we testing here?").
  2. Consistency
  3. Anything else

As (1) is the main priority, I would shy away from sacrificing it for stylistic reasons.

In this specific case, we are testing that it "accepts extra-files". We are not concerned with whether it can be a list, whether it accept globs, etc. (those aspects are covered by other test cases).

seemed a little 'circular' as an output

Yes, definitely agree!

However, if you would want to address this, then I think the right approach would be to get rid of the describe "extra-files".

But then again we already have things like:

describe "spec-version" $ do
  it "accepts spec-version" $ do

So keeping the describe is at least consistent with what we already have (in line with our second priority).

Copy link
Owner

Choose a reason for hiding this comment

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

@mpilgrem did you read this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, my mistake. I'll fix it now.

README.md Outdated
@@ -143,6 +143,7 @@ library:
| `build-type` | · | `Simple`, or `Custom` if `custom-setup` exists | Must be `Simple`, `Configure`, `Make`, or `Custom` | | |
| `extra-source-files` | · | | Accepts [glob patterns](#file-globbing) | | |
| `extra-doc-files` | · | | Accepts [glob patterns](#file-globbing) | | `0.21.2` |
| `extra-files` | · | | Accepts [glob patterns](#file-globbing) | | `0.39.0` |
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
| `extra-files` | · | | Accepts [glob patterns](#file-globbing) | | `0.39.0` |
| `extra-files` | · | | Accepts [glob patterns](#file-globbing) | | `0.38.1` |

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason I've held off on the Hpack version is that there may be other changes soon that could be released together (and extra-files on its own is not a burning platform) - I'm currently (off repository) looking at moving #518 forward.

Copy link
Owner

Choose a reason for hiding this comment

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

I say "release early, release often" and "minimize dependencies between tasks".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noted, but in this case #518 is on the heals of this one - and maybe #605 too, if I can convince you of its utility.

@mpilgrem
Copy link
Collaborator Author

Rebuilding unnecessarily was given as the motivation for extra-files:

but the fact it was not implemented for the first 20 years of Cabal suggests its need was not pressing!

@mpilgrem mpilgrem merged commit 48986d0 into main Mar 14, 2025
10 checks passed
@mpilgrem mpilgrem deleted the fix602 branch March 14, 2025 22:41
mpilgrem added a commit that referenced this pull request Mar 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants