Add and use MANIFEST.SKIP file for Porting files #19542
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add a new convenience option to manisort for a common case manisort --fix=MANIFEST_FILE is the same as manisort --nocheck --output=MANIFEST_FILE MANIFEST_FILE Eg, it runs quietly, returns an exit code of 0 on success and rewrites the file in correctly sorted order while removing true dupes.
The --fix option returns true if it worked, and so the use of the true command is not required.
Even though these pods are dead we should keep their descriptions for the manifest.
The comment says the highest exit code we should return is 124, but the code will return 125 if there are 125 problems, if it is higher than 125 it will return 124. Which doesn't make sense. This patch changes the logic to do what the comment says, return 124 if there are 124 or more problems, otherwise it returns the count of problems.
…nfra
This file is intended to list all the files in the repo which are not
listed in the main MANIFEST file, and which are used only for
development purposes, especially those files which are only useful when
working in a git checkout of the main perl git repository.
The files it contains will NOT be added to the production tarball
release. The file has the exact same format as the main MANIFEST:
"file\t+description" or "file".
Q. Why didn't I call this Porting/MANIFEST as mentioned in the
discussion thread that lead to this patch?
A. The main reason was that Porting/README.pod includes a list of files
in Porting with descriptions and explanations for what the files do
or how they are used. In several places the file refers to
"MANIFEST", which lead to ambiguity that would have had to be
resolved by changing all the entries to refer to "Porting/whatever"
instead. It was much simpler to give the new file an extension, and I
thought that '.dev' suggests it is for "development" purposes.
Q. Why isn't this using MANIFEST.skip style functionality?
A1. Various parts of our build and test process expect to read the
MANIFEST file and then do things based on the entries contained
within. Eg, run tests, or extract data, or compare the file list to
content in another file. Those parts of our build process would
break if we used a skip style list of regexen. So it would be more
work to teach them to deal with such a file, assuming it was
actually doable - given the additional work I have not considered it
deeply. On the other hand teaching that logic to simply read two
files was and is easy.
A2. I think each file we have in the repo should have a description.
This patch currently doesn't provide a description for each, but it
does for many, especially those migrated from MANIFEST.
A3. I think that MANIFEST.skip style files of exclusion regexens and
globs are error prone and easy to mess up, for instance by excluding
far more than you had intended to. They can also be annoying to get
right, obviously not impossible, but sometimes annoying. Explicitly
listing everything is easy in every way, especially to mechanize.
A4. I would like to be able to move verbatim entries from our existing
MANIFEST into the new Porting/MANIFEST.dev, description and all.
MANIFEST.skip style files do not support descriptions except as
comments as far as I recall. That would have meant munging the data
from MANIFEST during the move process which would be annoying.
A5. I would like to be able to reuse our sorting logic to keep the files
nicely sorted in a way where the file is somewhat readable. A list
of skip files would be less amenable to doing so.
Q. There is a lot of duplicated logic related to testing manifests,
should we refactor it out into a module or some resuable tool set?
A. YES! We already have Porting/manifest_lib.pm, but it currently does
not declare a package, and it only contains one function. Instead of
adding yet more code that depends on requiring a file and having it
inject subs into package main I decided that doing the refactoring
could wait for a separate commit or PR. But I definitely think we
should refactor as much of this logic as possible.
Q. Some of the test files were fairly significantly changed, are you
sure you didn't break or drop any of the tests?
A. I am reasonably confident I did not. Secondary review appreciated.
Some of the touched files are quite old and obviously "quick hack"
scripts. By rewriting them quite a bit I was able to simplify and
perform some of the tests in different ways or parts of the script.
As far as I know I didn't drop any.
Q. Why didn't you use newer features in the rewrite?
A. I am a bit conservative in my taste, and I like build tools to be
able to run on older perls, and for things like this I prefer to
stick with what I know well. Patches welcome.
Q. Why didn't you move more of the stuff we shouldn't bundle with
our releases?
A. I figured someone like Nicolas R. (who helped motivate this patch)
would feel left out if I didn't leave him anything to do. :-)
Patch best viewed with -w
Patch best viewed with -w
Patch best viewed with -w
Patch best viewed with -w
Patch best viewed with -w
Patch best viewed with -w
This replaces the lists of files to skip in Porting/manicheck, t/porting/manifest.t and Porting/makerel with a centralized and standardized MANIFEST.SKIP file. This would also allow us to add a make manifest target, but I left that for a follow-up PR. Signed-off-by: Nicolas R <[email protected]>
This change is merging the two ideas from #19523 and #19513 by relying on a traditional MANIFEST.SKIP file. Case #19523 simplifies the way we can easily exclude common noise Case #19513 provides a mechanism to avoid shipping not necessary files as part of the tarball This is adding a new 'Porting/Manifest.pm' file which provides helpers to list files from MANIFEST taking into account the 'MANIFEST.SKIP' using ExtUtils::Manifest.
This was referenced Mar 17, 2022
|
I think merging these branches into one bigger branch is the opposite of what we need here. This should have been at least three IMNSHO. |
|
note: I preserved your original commits and built this on top of your changes |
|
@atoomic, this p.r. has been inactive for over two years and has acquired numerous merge conflicts. It appears to me that we're not enthusiastic about applying it. Can it be closed? Thanks. |
jkeenan
added
the
Closable?
No response from OP or other commenters. Closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change is merging the two ideas from @demerphq #19513 and @Leont #19523
by relying on a traditional MANIFEST.SKIP file.
Case #19523 simplifies the way we can easily exclude common noise
Case #19513 provides a mechanism to avoid shipping not necessary
files as part of the tarball
This is adding a new 'Porting/Manifest.pm' file which provides helpers
to list files from MANIFEST taking into account the 'MANIFEST.SKIP'
using ExtUtils::Manifest.