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

Fix windows #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix windows #12

wants to merge 2 commits into from

Conversation

schwern
Copy link

@schwern schwern commented Aug 14, 2016

On a fresh Strawberry Perl 5.20.1 install (ie. AppVeyor) this is failing.

I found getting all the paths OS canonical a bit much, so I changed the tests. It's only human readable stuff and gets Windows passing.

I'm concerned that I couldn't find why this started failing. I see from CPAN Testers that it's been passing on Win32. Did something in the dependency chain change? Unfortunately CPAN Testers is erroring when I try to look at the passing results.

Rather than trying to get the code to use canonical paths,
I've changed the tests to go with what the code does.
This at least gets the tests working on Windows.
Your complimentary AppVeyor Windows CI config to test against
Strawberry Perl. Turn it on at appveyor.com.
schwern referenced this pull request in HayoBaan/Dist-Zilla-PluginBundle-Author-HAYOBAAN Aug 14, 2016
@karenetheridge
Copy link
Collaborator

karenetheridge commented Aug 14, 2016

Can you show me the output of the failing tests? There have been several win32-specific fixes that have gone in lately, which have seemed to satisfy everyone else, so I'm concerned that applying this change will cause a regression in other win32 systems. Let's instead figure out what's going on.

I know nothing about AppVeyor; what happens when this config file gets added?

@karenetheridge karenetheridge self-assigned this Aug 14, 2016
@schwern
Copy link
Author

schwern commented Aug 14, 2016

Sorry to forget the failure, and to explain AppVeyor. Both should be explained here.
https://ci.appveyor.com/project/schwern/p5-dist-zilla-plugin-run/build/1.0.1

AppVeyor is like Travis, but for Windows and not as well integrated with Github yet. The config provided does a CI test against Strawberry Perl 5.20.1.1 (just happens to be the easiest Perl version to get).

Like Travis, nothing will happen until you go to appveyor.com and turn it on. Unlike Travis, you have to configure Github notifications manually. http://www.appveyor.com/docs/notifications#github-pull-request

Since you're maintaining a huge chunk of the tool chain now, and CPAN Windows testing is notoriously poor, it would be great to have you start using it!

@schwern
Copy link
Author

schwern commented Aug 14, 2016

I'm not sure why Travis is failing. It passes here on OS X. OIC it never worked.

@rivy rivy mentioned this pull request Sep 2, 2016
@rivy
Copy link

rivy commented Sep 2, 2016

I'm encountering similar issues.

Upon digging in, I think the problem is that the "%d" parameter is not using portable path separators.

It appears that Dist::Zilla is using paths in "natural" form (i.e., unix-like, using '/' path separators), not in OS-canonical form. Given that, I think that the paths should be made canonical as late as possible.

There are three repair sites:

  1. For the "%d" parameter that would be in the +:::Run:Role::Runner::build_formatter() function.
  2. The positional parameter in +::Run::AfterBuild::after_build() would have to be corrected at it's construction site.
  3. And, lastly, $build_dir used the the test text within "./t/40_test_phase.t" would probably best be corrected at the point of test text construction.

I've made these repairs in PR#15 (which includes the changes introduced in PR#14 PR#16), for testing on travis-ci).

P.S. Sorry for all the commit noise. I was testing various configurations and didn't realize that the commit message reference would continually ad to the conversation. :\

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

Successfully merging this pull request may close these issues.

None yet

3 participants