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.

Sign up for GitHub

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

Jump to bottom

Add check for recursive glob in root directory #8441

Merged
merged 5 commits into from
Jan 18, 2023
Merged

Add check for recursive glob in root directory #8441

merged 5 commits into from
Jan 18, 2023

Conversation

dyniec
Copy link
Collaborator

@dyniec dyniec commented Aug 30, 2022 •
edited
Loading

Such globs might be expensive to include, as they might pull unnecessary
directories just like .git or dist-newstyle.
Solves #5311.
I would happily add tests for that, but I don't see any tests for warnings in cabal-check

Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

Tested manually with .cabal file that uses expressions **/*.ext in data-files, extra-source-files and extra-doc-files.

Warning: The following warnings are likely to affect your build negatively:
Warning: In the 'data-files' field: File glob **/*.dat starts at project root
directory this might include `.git/`, ``dist-newstyle/``, or other large
directories!
Warning: In the 'extra-source-files' field: File glob **/*.hs starts at
project root directory this might include `.git/`, ``dist-newstyle/``, or
other large directories!
Warning: In the 'extra-doc-files' field: File glob **/*.md starts at project
root directory this might include `.git/`, ``dist-newstyle/``, or other large
directories!
Warning: Hackage would reject this package.

ffaf1
ffaf1 reviewed Aug 31, 2022
View reviewed changes
Cabal/src/Distribution/PackageDescription/Check.hs Outdated
| (Left err, pat) <- zip globsExtraDocFiles $ extraDocFiles pkg
]
++
[ PackageBuildWarning $
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really a sintax error? Or a dangerous glob?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not a syntax error. It should probably go to different constructor

Copy link
Member

Choose a reason for hiding this comment

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

So, this is amended now? @ffaf1, could you perhaps give it another look?

Copy link
Member

Choose a reason for hiding this comment

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

@ffaf1: ping?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved it to separate constructor

@dyniec dyniec marked this pull request as ready for review September 1, 2022 21:25
@ffaf1 ffaf1 mentioned this pull request Sep 22, 2022
Merged
4 tasks
jneira
jneira approved these changes Jan 3, 2023
View reviewed changes
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.

this includes the changelog and test and looks fine, thanks!

ffaf1
ffaf1 reviewed Jan 3, 2023
View reviewed changes
Copy link
Collaborator

@ffaf1 ffaf1 left a comment

Choose a reason for hiding this comment

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

Very good.

Cabal/src/Distribution/Simple/Glob.hs Show resolved Hide resolved
Cabal/src/Distribution/Simple/Glob.hs Outdated Show resolved Hide resolved
@Mikolaj
Copy link
Member

Mikolaj commented Jan 11, 2023

@dyniec: when you are ready, please kindly set a squash label as in https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#github-pull-request-conventions.

@dyniec dyniec added the squash+merge me Tell Mergify Bot to squash-merge label Jan 11, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 13, 2023
@ulysses4ever
Copy link
Collaborator

@mergify refresh

@mergify
Copy link
Contributor

mergify bot commented Jan 15, 2023

refresh

✅ Pull request refreshed

@dyniec
Copy link
Collaborator Author

dyniec commented Jan 17, 2023

@mergify refresh

@mergify
Copy link
Contributor

mergify bot commented Jan 17, 2023

refresh

✅ Pull request refreshed

@dyniec
Copy link
Collaborator Author

dyniec commented Jan 17, 2023

I did rebase changes on top of current master branch. According to Mergifyio/mergify#5055:

The error seems to occur when Mergify triggers a Branch Update action from the merge queue, when a GitHub workflow file has changed on main in the mean time.

So rebase should get rid of error:

Base branch update has failed
refusing to allow a GitHub App to create or update workflow .github/workflows/bootstrap.yml without workflows permission

Changes are exactly the same, but since it was force-push it probably warrants some re-review?

@mergify mergify bot merged commit fff7a98 into haskell:master Jan 18, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Jan 18, 2023

Success! No, we don't re-review normally, unless the author prods us. Thank you very much for your contribution.

@ffaf1 ffaf1 mentioned this pull request Feb 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mikolaj Mikolaj Mikolaj left review comments

@ffaf1 ffaf1 ffaf1 approved these changes

@gbaz gbaz gbaz approved these changes

@jneira jneira jneira approved these changes

Assignees
No one assigned
Labels
cabal-install: cmd/check merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dyniec @Mikolaj @ulysses4ever @jneira @gbaz @ffaf1