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 #201 #202

Merged
merged 1 commit into from Jan 14, 2017
Merged

fix #201 #202

merged 1 commit into from Jan 14, 2017

Conversation

amba
Copy link
Contributor

@amba amba commented Jan 2, 2017

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 63.067% when pulling ad0bda0 on amba:fix-5.14 into b6abd2a on PDLPorters:master.

@mohawk2
Copy link
Member

mohawk2 commented Jan 2, 2017

Thanks for this! Please could you:

  • make the whitespace be consistent with the surrounding text (spaces not tabs)
  • help me understand how this is needed for <=5.14, when there is a build done for 5.10
  • help me understand why you're adding the EU::PXS version to CONFIGURE_REQUIRES since as far as I know that's simply incorrect; the configure (building Makefile.PL) does not as far as I know need EU::PXS, but the build does
  • if I'm right on the above, not delete from the BUILD_REQUIRES

@mohawk2
Copy link
Member

mohawk2 commented Jan 2, 2017

To add to my last: #201 explains why I now agree re the second and third points. However, please could you execute the 1st and 4th points? :-) @amba

Also, the line in .travis.yml which installes a recent E::P is no longer
needed.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 63.067% when pulling ad3e781 on amba:fix-5.14 into b6abd2a on PDLPorters:master.

@amba
Copy link
Contributor Author

amba commented Jan 3, 2017

@mohawk2, Thanks for the quick reply!

The indentation is fixed now.

Why should we duplicate the E::P requirement in BUILD_REQUIRES? From CPAN::Meta::Spec#Phases it seems that config requirements automatically apply to the build phase also.

@mohawk2
Copy link
Member

mohawk2 commented Jan 3, 2017

I think I experienced an issue with that in Gimp. However, I'm (very) pleased to be wrong. Seems like all my issues were either answered or wrong, and this is now good to go.

@devel-chm if you agree, want to merge? @amba If you feel that this would fix #201, want to use a fancy GH feature and edit either the PR title or opening comment to say "fix #201"?

@amba amba changed the title Fix build on perl <= 5.14 (see issue 201) fix #201 Jan 4, 2017
@devel-chm
Copy link
Member

I can't figure out how to get the fix from github to apply to the sf.net repo. Is there a way to get the change as a patch that could be applied to sf.net pdl master?

@mohawk2
Copy link
Member

mohawk2 commented Jan 8, 2017

That's a good question. If you look on the "Merge pull request" section of this page, you'll see a link to "view command line instructions". The title is "Merging via command line", and the commands I believe you'll want are:

git checkout -b amba-fix-5.14 master # make a local branch
git pull git://github.com/amba/pdl.git fix-5.14 # pull from the right branch of @amba's repo onto that local branch
git checkout master # back to your local master
git merge --no-ff amba-fix-5.14 # merge his branch onto your master
git push origin master # you might need to change "origin" to the remote you have for SF

@amba
Copy link
Contributor Author

amba commented Jan 11, 2017

Shell I provide a patch also? @mohawk2 @devel-chm

@amba amba closed this Jan 11, 2017
@amba amba reopened this Jan 11, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 63.067% when pulling ad3e781 on amba:fix-5.14 into b6abd2a on PDLPorters:master.

@mohawk2
Copy link
Member

mohawk2 commented Jan 12, 2017

@amba That shouldn't be necessary. @devel-chm any problems with the ideas above?

@devel-chm
Copy link
Member

The last time I tried to rebase the fix to the lastest master there were a number of merge conflicts which I haven't had time to look at or to figure out if I was doing something wrong. I should have some more time no later than this weekend. Of course with a ff-only compliant patch this would be done in a jiffy.

@wchristian
Copy link
Member

I have no idea what conflicts you could possibly run into, as amba's commit already is fully ff-only compliant.

image

@devel-chm
Copy link
Member

Thanks for the pointers. I got confused with which master was which...

@wchristian wchristian merged commit ad3e781 into PDLPorters:master Jan 14, 2017
@devel-chm
Copy link
Member

appveyor build is failing for strawberry perl.

@wchristian
Copy link
Member

That looks to be an issue unrelated to this one, as it's some kind of tool for installing strawberry that's failing because of missing checksums.

@amba amba deleted the fix-5.14 branch January 14, 2017 18:29
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

5 participants