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

Pass explicit mirror list including BackPAN to cpan #2135

Closed
wants to merge 1 commit into from

Conversation

scop
Copy link
Contributor

@scop scop commented Nov 22, 2021

Gives out of the box ability to use dependencies that have been archived to BackPAN.

Refs #2134

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

Gives out of the box ability to use dependencies that have been archived
to BackPan.

Refs pre-commit#2134
@asottile
Copy link
Member

@scop hmm, seems to have broken windows

@scop
Copy link
Contributor Author

scop commented Nov 22, 2021

That I have no explanation for, nor a system to try it out with, and the logs and the error message aren't helpful on first sight. Kind of makes me wonder if if's related to this change at all. Maybe something's cleaning up temp dirs too early 🤷‍♂️

@asottile
Copy link
Member

the failure mode is different from the other PR so I'm pretty sure this changed something

looking at the other failure on windows I wonder if cpan doesn't propagate exit codes properly on windows since it appears to be continuing along even when the install should be failing

@asottile
Copy link
Member

for now I bumped the version to get it green again -- though we should probably merge this or something like it at some point once we work out what's broken on windows

@scop
Copy link
Contributor Author

scop commented Nov 23, 2021

Random thoughts/guesswork:

Would it be possible to capture and have a look at cpan's stdout/stderr? ISTR there wesn't anything builtin in pre-commit for that, but I might be mistaken on this. If there's not, I think it would be a useful addition.

Do we know what version of Perl runs in CI? -M was added in cpan 2.12ish, beginning of 2016, Perl 5.25.2 is the first one that comes with that or later out of the box. If the "too new" version is a problem, there's probably another more laborous way to set up the mirrors that works with older versions, by writing a config file.

Maybe we should try with plain http instead of https for the mirrors. Some vague memories of working https not being a given long time ago with Windows and Perl.

@asottile
Copy link
Member

yeah cmd_output already does that -- it's currently discarded but you can debug with it if you need to

the versions of both cpan and perl are newer than that from the github virtual environments docs

@asottile
Copy link
Member

I worked around this in another way -- thanks for the patch though!

@asottile asottile closed this Dec 24, 2021
@scop
Copy link
Contributor Author

scop commented Dec 25, 2021

I'd be interested to hear more about the workaround, didn't spot anything related in recent commit logs here.

Also, I think this change would still be a desirable one to merge, as users are likely to run into similar issues that this was originally intended as a fix for within pre-commit's own tests.

@asottile
Copy link
Member

I just bumped the version -- mostly kicking the can down the road. I agree this would still be good to add, though the windows breakage has me concerned :(

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

Successfully merging this pull request may close these issues.

None yet

2 participants