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.

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

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

Open
jackdeguest opened this issue Aug 22, 2023 · 8 comments

Comments

@jackdeguest
Copy link

jackdeguest commented Aug 22, 2023

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.

@jackdeguest jackdeguest changed the title Bug with manicopy on MacOS Ventura (13.5) Bug with manicopy linking files instead of copying them when making distdir Aug 22, 2023
@haarg
Copy link
Member

haarg commented Aug 22, 2023

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

@jackdeguest
Copy link
Author

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
Copy link
Member

Leont commented Aug 22, 2023

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
Copy link
Author

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
Copy link
Member

Leont commented Aug 22, 2023

Patches welcome?

@jackdeguest
Copy link
Author

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
Copy link
Member

haarg commented Aug 23, 2023

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
Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants