Skip to content

Add MANIFEST.SKIP file #19523

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
Open
wants to merge 1 commit into
base: blead
Choose a base branch
from
Open

Add MANIFEST.SKIP file #19523

wants to merge 1 commit into from

Conversation

Leont
Copy link
Contributor

@Leont Leont commented Mar 11, 2022 •
edited
Loading

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.

@leonerd
Copy link
Contributor

leonerd commented Mar 11, 2022

This seems a good idea, as it's already a mechanism familiar to CPAN authors, being the standard way those dists usually work.

@Leont Leont mentioned this pull request Mar 11, 2022
Open
atoomic
atoomic approved these changes Mar 11, 2022
View reviewed changes
Copy link
Member

@atoomic atoomic left a comment

Choose a reason for hiding this comment

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

Leon++

@atoomic
Copy link
Member

atoomic commented Mar 11, 2022

need to sync with #19513 effort
IMO we should merge this one first

atoomic
atoomic reviewed Mar 11, 2022
View reviewed changes
atoomic
atoomic reviewed Mar 11, 2022
View reviewed changes
@atoomic
Copy link
Member

atoomic commented Mar 11, 2022

After spending more time reading the PR for MANIFEST.SKIP and MANIFEST.DEV from #19523 and #19513

Here is my thought:

  • I prefer the syntax of MANIFEST.SKIP this makes it so much simpler for example to skip the .gitignore files everywhere
  • The PR with the MANIFEST.DEV is updating a few extra tools

I m currently working on a prototype to reconcile both branches and keep the best from each

@Leont
Copy link
Contributor Author

Leont commented Mar 12, 2022

After spending more time reading the PR for MANIFEST.SKIP and MANIFEST.DEV from #19523 and #19513

Here is my thought:

* I prefer the syntax of `MANIFEST.SKIP` this makes it so much simpler for example to skip the `.gitignore` files everywhere

* The PR with the MANIFEST.DEV is updating a few extra tools

I m currently working on a prototype to reconcile both branches and keep the best from each

I don't think this branch needs anything from that other branch to be complete. I do think this branch is asking for a follow up, but I'm not sure yet what that follow up should look like.

But most of all, I think we need to agree on requirements before changing any behavior. Because arguing about an implementation when we're actually disagreeing about requirements leads to very muddy discussions.

@atoomic
Copy link
Member

atoomic commented Mar 12, 2022

I agree this branch can be safely merged as its own.
IMO we try to easily control what is ship and what is not packaged in the tarball.

This is a good start and we can do some incremental changes

I'm all in to press the merge button :-) there

@demerphq
Copy link
Collaborator

demerphq commented Mar 12, 2022 via email

@atoomic
Copy link
Member

atoomic commented Mar 12, 2022

I agree with @demerphq both PRs are solving two different problems.

To be honest I would like the best of the two. I'm not sure it's easily doable but definitively worth the try.
As @demerphq just showed above moving an explicit list of files to some rules... is going to require some unit tests adjustments.

I do not think we should use File::Find for listing porting files but probably prefer a git ls-files with the manifest.skip

For example we could provide some shared helper to list all the porting files: get_porting_files, ....
Then tools and test could rely on that to get the list of porting/* files
This could be a no go if we need to check some generated files not git versioned. But I'm not sure such files exist.

##
## list of all files from MANIFEST and ignored by MANIFEST.SKIP
##

# note: need to temporary chdir to the root directory 
sub get_files_from_all_manifests {

    local $ExtUtils::Manifest::Quiet = 1;

    require ExtUtils::Manifest;

    my @from_manifest;# = keys %{ ExtUtils::Manifest::maniread("MANIFEST") };
    my @from_manifest_skip;

    my $skip = ExtUtils::Manifest::maniskip( "MANIFEST.SKIP" );

    my @ls_files = `git ls-files --full-name`;
    die q[Fail to run git ls-files] if $?;
    chomp(@ls_files);

    foreach my $f ( @ls_files ) {
        next unless $skip->($f);
        push @from_manifest_skip, $f;
    }

    my %uniq = map { $_ => 1 } @from_manifest, @from_manifest_skip;

    return ( sort keys %uniq );
}

##
## list of Porting files listed in MANIFEST or ignored by MANIFEST.SKIP
##

sub get_porting_files {
    return grep { qr{^Porting} } get_files_from_all_manifests();
}

Sum-up:
Could we use MANIFEST.SKIP to exclude all the internal / tools and at the same time rely on it for our need to check the porting files (test & tools)? I'm not sure, but I think it worth the try. We would have cleaner syntax and easy way to exclude files. We could at the same time simplify code trying to extract content from the MANIFEST.

atoomic added a commit that referenced this pull request Mar 17, 2022
@atoomic
Add and use MANIFEST.SKIP file for Porting files
b232f21 
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.
@atoomic atoomic mentioned this pull request Mar 17, 2022
Closed
@atoomic
Copy link
Member

atoomic commented Mar 17, 2022

Note: #19542 provides one suggestion to merge both ideas from #19513 and #19523

@khwilliamson
Copy link
Contributor

What should be done about this PR?

@atoomic
Copy link
Member

atoomic commented Jun 8, 2022

IMO we should close this PR, but I'm sure other persons can have mixed feelings.

@Leont
Copy link
Contributor Author

Leont commented Jun 10, 2022

What should be done about this PR?

I think it's up to the PSC to decide what to do with this.

@jkeenan jkeenan added the Infrastructure Things needed to maintain Perl development label Jun 28, 2022
@jkeenan
Copy link
Contributor

jkeenan commented Jan 17, 2023

What should be done about this PR?

I think it's up to the PSC to decide what to do with this.

This p.r. for adding a MANIFEST.SKIP file to the core distribution has languished since July and has a small merge conflict. It received one reviewer's Approval but then attracted dissents. @Leont, if you want to move this forward, can you resolve the merge conflict? After that, I propose we give the PSC two weeks to make a ruling on this p.r.
@rjbs, @book, @leonerd

@Leont
Add MANIFEST.SKIP file
ad8f2a2 
This replaces the lists of files to skip in Porting/manicheck,
t/porting/manifest.t 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atoomic atoomic atoomic approved these changes

Assignees
No one assigned
Labels
hasConflicts Infrastructure Things needed to maintain Perl development
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Leont @leonerd @atoomic @demerphq @khwilliamson @jkeenan