Added Perl #161

Closed
wants to merge 24 commits into
from

Projects

None yet

2 participants

@pjlsergeant

Perl language added. Please let me know what I can do to make it better.

I currently own the permissions on CPAN to the Gherkin namespace. Please sign up at least one official account and I'll transfer ownership over.

Owner

This looks great! I'll try to take a deeper dive later this week.

Thanks!!

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Feb 24, 2016
perl/Makefile
+ diff --unified $<.ast.json $@
+.DELETE_ON_ERROR: acceptance/testdata/%.feature.ast.json
+
+acceptance/testdata/%.feature.pickles.json: ../testdata/%.feature ../testdata/%.feature.pickles.json .built
+ mkdir -p `dirname $@`
+ bin/gherkin-generate-pickles $< > $@
+ diff --unified $<.pickles.json $@
+.DELETE_ON_ERROR: acceptance/testdata/%.feature.pickles.json
+
+acceptance/testdata/%.feature.errors: ../testdata/%.feature ../testdata/%.feature.errors .built
+ mkdir -p `dirname $@`
+ ! bin/gherkin-generate-ast $< 2> $@
+ diff --unified $<.errors $@
+.DELETE_ON_ERROR: acceptance/testdata/%.feature.errors
+
+gherkin/gherkin-languages.json: ../gherkin-languages.json
aslakhellesoy
aslakhellesoy Feb 24, 2016 Owner

This target fails with:

cp ../gherkin-languages.json gherkin/gherkin-languages.json
cp: gherkin/gherkin-languages.json: No such file or directory
make: *** [gherkin/gherkin-languages.json] Error 1

I think it would be better if helper-scripts/build_languages.pl read ../gherkin-languages.json directly (via STDIN). No need to copy the file if it's just a temporary copy.

pjlsergeant
pjlsergeant Feb 24, 2016

That's a good idea, I'll do that

@aslakhellesoy aslakhellesoy commented on the diff Feb 24, 2016
perl/CONTRIBUTING.md
@@ -0,0 +1,16 @@
+Please read [CONTRIBUTING](https://github.com/cucumber/gherkin/blob/master/CONTRIBUTING.md) first.
+You should clone the [cucumber/gherkin](https://github.com/cucumber/gherkin) repo if you want
+to contribute.
+
+You will need `cpanm` installed on your system
aslakhellesoy
aslakhellesoy Feb 24, 2016 Owner

In that case, the CI server will need it too. Please update /.travis.yml (add an apt package I suppose)

pjlsergeant
pjlsergeant Feb 24, 2016

I had some questions about this...

The tests rely on being able to find the parent test-data stuff, which - I think - makes a perl/.travis file unfeasible. Should I simply add to the main project travis file? The cpanm dep is easy to add to travis

aslakhellesoy
aslakhellesoy Feb 24, 2016 Owner

Yes, please add it to the root /.travis.yml file.

@aslakhellesoy aslakhellesoy commented on an outdated diff Feb 24, 2016
@@ -0,0 +1,168 @@
+Revision history for perl module Gherkin.pm, and the upstream Gherkin project. This file is automatically generated, don't edit by hand. Instead edit the CPAN-ONLY-CHANGES file in the git repo.
aslakhellesoy
aslakhellesoy Feb 24, 2016 Owner

I'd rather not have to maintain build scripts to create a modified copy CHANGES for every implementation. It's also harder for developers to have many CHANGES files to maintain. Can we remove this file?

@aslakhellesoy aslakhellesoy commented on an outdated diff Feb 24, 2016
perl/CPAN-ONLY-CHANGES
@@ -0,0 +1,15 @@
+This file contains changes in the CPAN-only releases of this module. Its entries
aslakhellesoy
aslakhellesoy Feb 24, 2016 Owner

As mentioned above it would simplify our process a lot if we don't have to maintain this file.

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Feb 24, 2016
perl/LICENSE.txt
@@ -0,0 +1,24 @@
+The MIT License (MIT)
aslakhellesoy
aslakhellesoy Feb 24, 2016 Owner

There should only be one master LICENSE file, which is copied to all implementations with Make. Feel free to add your name to the parent LICENSE file.

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Feb 24, 2016
perl/dist.ini
@@ -0,0 +1,48 @@
+name = Gherkin
+abstract = A parser and compiler for the Gherkin language
+main_module = lib/Gherkin.pm
+author = ['Peter Sergeant <pete@clueball.com>', 'Gaspar Nagy']
aslakhellesoy
aslakhellesoy Feb 24, 2016 Owner

Please add Cucumber Ltd as one of the authors - should be same as in /LICENSE

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Feb 24, 2016
perl/dist.ini
+abstract = A parser and compiler for the Gherkin language
+main_module = lib/Gherkin.pm
+author = ['Peter Sergeant <pete@clueball.com>', 'Gaspar Nagy']
+license = MIT
+is_trial = 0
+perl = 5.008
+copyright_holder = Peter Sergeant, Gaspar Nagy
+
+; Worth seeing the note about this in Gherkin.pm. You are aiming for the Gherkin
+; version with the minor revision %2d and a two-digit CPAN version added. For
+; example:
+;
+; 3.1.2 -> 3.1.0201, 3.1.0202, 3.1.0203 ... etc
+; 3.2.0 -> 3.2.0001, 3.2.0002 ... etc
+;
+; CPAN releases can't be changed, and this allows a nice simple way of uploading
aslakhellesoy
aslakhellesoy Feb 24, 2016 Owner

I doubt we'll make emergency releases. We haven't in the past. We need a more streamlined release process now that there are 8 implementations of Gherkin.

pjlsergeant
pjlsergeant Feb 24, 2016

I note that the Gherkin release I've made is now on .0004. While this is due to incompetence on my part, I do wonder about this moving forward. CPAN is particularly unforgiving about allowing rereleases.

The problem I envisage is that you guys make a new Gherkin release, and for some Perl-related peculiarity, it breaks tests on certain machines. This then means it can't be installed on those machines. Do you really want to do a whole new central Gherkin release because the Perl version can't be installed on 50% of the machines due to some Perl weirdery?

In terms of the release process, running make release should transparently take care of all of the CHANGES related stuff, although as you correctly point out, that's an extra file to maintain.

aslakhellesoy
aslakhellesoy Feb 24, 2016 Owner

CPAN is particularly unforgiving about allowing rereleases.

Almost all package repositories are, and I think that is a good thing.

Do you really want to do a whole new central Gherkin release because the Perl version can't be installed on 50% of the machines due to some Perl weirdery?

Our release cycle is currently roughly every month. Between each release there are usually several changes across platforms, so I don't see this as a problem. If there are bugs in the perl implementation we'll fix them, but it probably won't affect the release cycle. People who can't wait will have to grab git master :-)

pjlsergeant
pjlsergeant Feb 24, 2016

OK, I can live with that

@aslakhellesoy aslakhellesoy commented on the diff Feb 24, 2016
perl/bin/gherkin-generate-ast
@@ -0,0 +1,32 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+use lib 'lib';
+
+use Cpanel::JSON::XS;
aslakhellesoy
aslakhellesoy Feb 24, 2016 Owner

I'm getting this error:

Can't locate Cpanel/JSON/XS.pm in @INC (you may need to install the Cpanel::JSON::XS module) (@INC contains: lib /Library/Perl/5.18/darwin-thread-multi-2level /Library/Perl/5.18 /Network/Library/Perl/5.18/darwin-thread-multi-2level /Network/Library/Perl/5.18 /Library/Perl/Updates/5.18.2/darwin-thread-multi-2level /Library/Perl/Updates/5.18.2 /System/Library/Perl/5.18/darwin-thread-multi-2level /System/Library/Perl/5.18 /System/Library/Perl/Extras/5.18/darwin-thread-multi-2level /System/Library/Perl/Extras/5.18 .) at bin/gherkin-generate-ast line 7.
BEGIN failed--compilation aborted at bin/gherkin-generate-ast line 7.

This is because the .built target depends on .cpanfile_dependencies after running this script - and that's too late.

pjlsergeant
pjlsergeant Feb 24, 2016

I'll sort that, thanks

Owner

This is really solid! The next step would be to make Travis build this. When it's green and all comments in here are addressed I'll merge this in.

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Feb 24, 2016
perl/Makefile
+ mkdir -p `dirname $@`
+ bin/gherkin-generate-pickles $< > $@
+ diff --unified $<.pickles.json $@
+.DELETE_ON_ERROR: acceptance/testdata/%.feature.pickles.json
+
+acceptance/testdata/%.feature.errors: ../testdata/%.feature ../testdata/%.feature.errors .built
+ mkdir -p `dirname $@`
+ ! bin/gherkin-generate-ast $< 2> $@
+ diff --unified $<.errors $@
+.DELETE_ON_ERROR: acceptance/testdata/%.feature.errors
+
+gherkin/gherkin-languages.json: ../gherkin-languages.json
+ cp $^ $@
+
+release: .compared CHANGES
+ cp ../testdata/good/*.feature acceptance/testdata/good/
aslakhellesoy
aslakhellesoy Feb 24, 2016 Owner

Why are you copying these files over?

pjlsergeant
pjlsergeant Feb 24, 2016

Because they end up forming the basis of the install tests. CPAN modules tend to run comprehensive tests on install - they've already caught a few platform specific things I'd missed. It would be a real shame (and very unPerlish) to lose these and not distribute them with the package.

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Feb 24, 2016
perl/CONTRIBUTING.md
+Please read [CONTRIBUTING](https://github.com/cucumber/gherkin/blob/master/CONTRIBUTING.md) first.
+You should clone the [cucumber/gherkin](https://github.com/cucumber/gherkin) repo if you want
+to contribute.
+
+You will need `cpanm` installed on your system
+
+## Run tests
+
+### Using make
+
+Just run `make` from this directory.
+
+## Make a release
+
+ # Change version in `dist.ini` and run
+ make release
aslakhellesoy
aslakhellesoy Feb 24, 2016 Owner

It's nice that this is automated!

pjlsergeant
pjlsergeant Feb 24, 2016

Should be able to automate upload too very easily, to save people having to log in to PAUSE.

Peter Sergeant added some commits Feb 25, 2016
Peter Sergeant Fix issues mentioned in cucumber#161 991096c
Peter Sergeant Bump VERSION in anticipation of next release a3d23b2
Peter Sergeant Fix .travis.yml for Perl, added information on CPAN trial releases 6d1aeec
Peter Sergeant Fix Travis, again, add better release builder 858346d
Peter Sergeant Remove unused dependencies, including one failing one dca48d6
Peter Sergeant Third time the charm for the .travis.yml? f684886
Peter Sergeant Further attempt to fixing Perl's Travis job
Also moved it to the front of the queue as it's currently most
unstable...
94bb626
Peter Sergeant Added the generated Parser back in
This stops Travis from trying to regenerate it, and brings this inline
with the other languages.

Small fix from before_install -> install
9f8a953

OK, I think I have addressed all of your concerns here. Now passing in Travis.

HOWEVER, note that I have added a VERSION file to the parent, which this gets its version from. It seems to me that ultimately all languages should be reading off the same script in that regard. Let me know if this isn't something you're happy with, and I guess I'll just put it in the Perl version for now.

Owner

I've registered user HELLESOY on PAUSE. Do you need anything else to give me release karma?

No, that's all transferred now, and you are the primary maintainer. I remain as a co-maintainer, which means I am also able to make authorized releases. You can add further co-maintainers (or indeed remove me as one) via https://pause.perl.org/

My recommendation is you should ask me to make releases, as the releaser gets all sorts of status emails, CPAN RT emails, CPAN Testers reports (that need looking at), and an apparently endless stream of complaints about spelling mistakes from the Debian maintainers :-P

Owner

Ha! Will do. I'll ping you every time we cut a release :-)

@aslakhellesoy aslakhellesoy added a commit that referenced this pull request Feb 25, 2016
@aslakhellesoy Peter Sergeant + aslakhellesoy Fix issues mentioned in #161 7003143
@aslakhellesoy aslakhellesoy added a commit to cucumber/gherkin-perl that referenced this pull request Feb 25, 2016
@aslakhellesoy Peter Sergeant + aslakhellesoy Fix issues mentioned in cucumber/gherkin#161 33f1a18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment