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

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

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

Conversation

ehuelsmann
Copy link
Contributor

Fixes #32

🤔 What's changed?

The Languages.pm file is now generated upon building the release artifacts or when running the test suite.

⚡️ What's your motivation?

Issue #32 (Languages.pm not mergeable)

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, documentation etc. without changing behaviour)

♻️ Anything particular you want feedback on?

Does this address the concern sufficiently?

📋 Checklist:


This text was originally generated from a template, then edited by hand. You can modify the template here.

[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

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.

@ehuelsmann
Copy link
Contributor Author

The latest Iteration of this PR generates Languages.pm before testing and release artefact building. It does that by extending the dzil build command. The tests are being run based on what is in the git tree (as they always have). I

f we want to run the tests based on what is in the release tarball, we need to install its content. dzil test and dzil run exists for that purpose: they create an out of tree isolated installation as would be achieved with a vanilla installation. Using dzil this way is not possible, because our testdata needs the tests to run in a relative path to the test data set.

Looking at how Ruby does it, I see it uses bundler . Although I'm not too much into the details, I think the description of what we do now (run from the working tree) is exactly what Ruby does too, since bundle package isn't required for bundle exec and the latter is what the tests use.

Concluding: I think this PR brings the Perl situation in line with what Ruby does.

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.

perl: Languages.pm is not mergable
2 participants