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.

Sign up for GitHub

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

Don't maintain a generated Languages.pm file #176

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Don't maintain a generated Languages.pm file #176

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9fd1084
Don't maintain a generated Languages.pm file
ehuelsmann Sep 24, 2023
63a22b5
Use 'dzil build' to build a complete Gherkin-{VERSION} lib directory
ehuelsmann Oct 13, 2023
48d8e26
Use the generated languages file in the source tree
ehuelsmann Oct 13, 2023
730a3e1
Make 'dzil' discoverable
ehuelsmann Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions .github/workflows/test-perl.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,13 @@ jobs:
install-modules-with: cpanm
working-directory: perl

# Run acceptance tests first! They build Languages.pm
- name: run acceptance tests
run: make acceptance
working-directory: perl

- name: run tests
run: |
prove -l
AUTHOR_TESTS=1 prove -l
working-directory: perl

- name: run acceptance tests
run: make acceptance
working-directory: perl
1 change: 1 addition & 0 deletions perl/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ perl5/
.cpanfile_dependencies
cpanfile.snapshot
CHANGELOG.md
lib/Gherkin/Generated/Languages.pm
2 changes: 1 addition & 1 deletion perl/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ clean: ## Remove all build artifacts and files generated by the acceptance tests

.DELETE_ON_ERROR:

acceptance: .built $(TOKENS) $(ASTS) $(PICKLES) $(SOURCES) $(ERRORS) ## Build acceptance test dir and compare results with reference
acceptance: .built lib/Gherkin/Generated/Languages.pm $(TOKENS) $(ASTS) $(PICKLES) $(SOURCES) $(ERRORS) ## Build acceptance test dir and compare results with reference

.built: perl5 $(SOURCE_FILES)
touch $@
Expand Down
8 changes: 7 additions & 1 deletion perl/dist.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,16 @@ exclude_filename=gherkin-perl.razor
exclude_filename=Makefile
exclude_filename=VERSION

[GatherFile]
[GatherFile / CHANGELOG.md]
; explicitly add unversioned files
root=../
filename=CHANGELOG.md

[GatherFile / GeneratedLanguages]
filename=lib/Gherkin/Generated/Languages.pm

[Hook::VersionProvider]
. = my $v = `cat ./VERSION`; chomp( $v ); $v;

[Run::BeforeBuild]
run = helper-scripts/build_languages.pl >lib/Gherkin/Generated/Languages.pm
Copy link
Contributor

@mpkorstanje mpkorstanje Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

With this in place, I would expect the code from the Makefile that does the generation is no longer needed. And with it some other changes can be removed too.

lib/Gherkin/Generated/Languages.pm:
	perl helper-scripts/build_languages.pl > $@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the Makefile targets are still used for the GitHub Actions CI runs. Those runs are specialized because they depend on the conformance data, which isn't packaged in the release tarball. I need to check again, but from what I remember dzil passes a cleaned environment to the test scripts, so the scripts needed a predictable directory to find the conformance data.

The dzil build target only packages the files for release. The dzil test target only runs a few syntax checks. Definitely not enough to validate conformance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the Makefile as a whole is still used.

But you've created an alternative for the generation of lib/Gherkin/Generated/Languages.pm. And I would generally expect a build process to have a single way of doing this. So I would not expect perl helper-scripts/build_languages.pl to be invoked in the Makefile anymore.

Perhaps dzil build would have to be invoked instead cpanm? And perhaps the acceptance tests would have to be ran with the artifact produced by dzil build?

perl5:
	cpanm --notest --local-lib ./perl5 --installdeps --with-develop .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running the tests with an environment as it would have been created by installing the artifact from dzil build , can be done using dzil test (to run the Perl test suite) or dzil run (to run an arbitrary command).

I guess what it comes down to is: should the Makefile follow the patterns all other Makefiles use (thereby deviating from "the Perl way")? Or should I use "the Perl way", thereby deviating from "the project way"?

I guess I know the answer: since Elixir doesn't do it the project way anyway, so there's precedent for doing it "the language's ecosystem way". I'll change this PR to de-duplicate accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I know what the "Perl way" is. ;)

But most languages do follow the pattern of executing the code in the artifact produced by the build system - occasionally with a shim.

For example JavaScript uses npm to build the artifact.

.built: node_modules dist $(SOURCE_FILES)
	touch $@

node_modules:
	npm install

dist:
	npm run build

And then to run the tests

GHERKIN = node ./test-cli.mjs

Where test-cli.mjs is a shim that uses Gherkin to run the acceptance tests.

Perl seems to differ mostly in that it uses both cpanm (to create the artifact used by tests) and dzil (to create the released artifact). I would prefer it if we have system for both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perl seems to differ mostly in that it uses both cpanm (to create the artifact used by tests)

Ah! There'sa misconception there. Good to be able to clarify that. cpanm is actually not building the artifact (dzil build is); cpanm is about installing the dependencies to be able to run the tests (as well as dzil itself). Until this PR (which removes lib/Gherkin/Generated/Languages.pm) the tests can be run without any build steps straight out of the repository.

This PR changes that, because with this PR a build step is required (generating the missing languages file) before the tests can be run. This is an extra step being introduced in this PR.

My plan now is to change the acceptance Makefile target: it will invoke dzil test, which will generate the missing languages file. I will also change the acceptance/* targets to Perl code which can be run as part of dzil test (which assumes it can run the files in t/ as Perl test files; it doesn't invoke the project's Makefile).

Copy link
Contributor

Choose a reason for hiding this comment

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

I will also change the acceptance/* targets to Perl code which can be run as part of dzil test

This part has me a little confused. The way the acceptance tests are run seems correct to me.

Until this PR (which removes lib/Gherkin/Generated/Languages.pm) the tests can be run without any build steps straight out of the repository.

Then I would now expect perl helper-scripts/build_languages.pl to be replaced with dzil build or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part has me a little confused. The way the acceptance tests are run seems correct to me.

They are as long as the file lib/Gherkin/Generated/Languages.pm exists, but this commit removes that file. Instead, I'm invoking dzil build, which generates a release tarball, but Perl can't use that tarball to load its modules from. It also generates a directory with the content of that tarball, but that has the version number embedded (making the Makefile a more complex).

To keep the makefile simple, I was going to go with the way Elixir works, but that breaks down for other reasons (the URI value).

I've taken yet another direction with this PR now, based on your comments and my findings so far.

7 changes: 0 additions & 7 deletions perl/lib/Gherkin/Generated/Languages.pm
View file
Open in desktop

This file was deleted.