Perl-Toolchain-Gang / ExtUtils-Manifest

Utilities to write and check a MANIFEST file
https://metacpan.org/release/ExtUtils-Manifest/
Other
4 stars 7 forks source link

Bug with manicopy linking files instead of copying them when making distdir #38

Open jackdeguest opened 11 months ago

jackdeguest commented 11 months ago

When executing the Makefile command make distdir, this calls the command create_distdir which uses ExtUtils::Manifest manicopy. It goes on to create a distribution directory, and for some obscure reason, the module files within this distribution directory seem linked to their originals. Indeed, when modifying the copied module, it also modifies the original one. Reverting the change made in the original also modifies the copy! It is as if the copy was not copied but hard linked to the original.

I doubt this should be the intended outcome if the PREOP option is set, because the procedure would modify both files.

To replicate the bug, take a perl distribution, open its main module in an IDE, then type perl Makefile.PL && make distdir, then edit the equivalent copied module file, and make some changes (e.g. change the version number), and you will see the same change also occurring in the original file.

Update: This is not related to MacOS and is actually more related to ExtUtils::MakeMaker that calls ExtUtils::Manifest.

haarg commented 11 months ago

You can pass MakeMaker the option dist => { DIST_CP => 'cp' } to avoid this behavior.

jackdeguest commented 11 months ago

You can pass MakeMaker the option dist => { DIST_CP => 'cp' } to avoid this behavior.

Thank you. I saw that. I think the default option should be cp if PREOP is used, otherwise there is a certain likelihood of problem like mine occurring.

I do not even understand why linking to files rather than plain copy.

Leont commented 11 months ago

Thank you. I saw that. I think the default option should be cp if PREOP is used, otherwise there is a certain likelihood of problem like mine occurring.

That could make sense, but I'm not sure it's really worth it for such a niche thing.

jackdeguest commented 11 months ago

That could make sense, but I'm not sure it's really worth it for such a niche thing.

That's one perspective. Another is the risk-impact, i.e. the damage this has the potential of doing. But, I can understand if it is difficult to code this consideration.

Leont commented 11 months ago

Patches welcome?

jackdeguest commented 11 months ago

Patches welcome?

Sure. Please find below my proposal. However, please note that I am not familiar with the code of this distribution, so there may need more adjustments.

*** /tmp/ExtUtils-MakeMaker-7.70/lib/ExtUtils//MM_Unix.pm   Sun Mar 26 22:13:13 2023
--- /tmp/MM_Unix-7.71.pm    Wed Aug 23 07:14:19 2023
***************
*** 633,639 ****

      $self->{CI}       ||= 'ci -u';
      $self->{RCS_LABEL}||= 'rcs -Nv$(VERSION_SYM): -q';
!     $self->{DIST_CP}  ||= 'best';
      $self->{DIST_DEFAULT} ||= 'tardist';

      ($self->{DISTNAME} = $self->{NAME}) =~ s{::}{-}g unless $self->{DISTNAME};
--- 633,639 ----

      $self->{CI}       ||= 'ci -u';
      $self->{RCS_LABEL}||= 'rcs -Nv$(VERSION_SYM): -q';
!     $self->{DIST_CP}  ||= ( $self->{PREOP} eq '$(NOECHO) $(NOOP)' ? 'best' : 'cp' );
      $self->{DIST_DEFAULT} ||= 'tardist';

      ($self->{DISTNAME} = $self->{NAME}) =~ s{::}{-}g unless $self->{DISTNAME};
haarg commented 11 months ago

Pretty sure that won't work, because PREOP is an argument to dist and doesn't store its value in $self.

I'm not sure that providing a PREOP option is really a useful signal for if the files in the dist dir are going to be modified. Most users use it to generate a README file. I can't tell what your use case is because you don't ship your cleanup script nor have it in your repositories.

You could also have your script modify the files by generating new files and renaming them over the existing files, rather than editing them in place.

jackdeguest commented 11 months ago

Pretty sure that won't work, because PREOP is an argument to dist and doesn't store its value in $self.

I just copied what was in the original module...

You could also have your script modify the files by generating new files and renaming them over the existing files, rather than editing them in place.

But why would I go to such trouble, when the advertised PREOP option allows me to change the files before the operation?