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

Upgrade to berp 1.3, remove dependency to mono #1542

Merged
merged 27 commits into from
May 19, 2021
Merged

Conversation

gasparnagy
Copy link
Member

Upgrading Berp to a version runs on .NET 5.0

@gasparnagy gasparnagy marked this pull request as draft May 14, 2021 13:58
@aslakhellesoy
Copy link
Contributor

Is this good to merge now? LGTM!

@gasparnagy
Copy link
Member Author

@aslakhellesoy no, this is now only fixes it for dotnet. But a review is welcome. If you are fine with the approach, i will replicate it to the other languages.

@gasparnagy
Copy link
Member Author

@aslakhellesoy what do you think about creating a shared.mk file that is a single file in the templates, but it is rsynced to all language folders. That way we could reduce the duplications in the language specific default.mk files...

@aslakhellesoy
Copy link
Contributor

We could, but it might come at the cost of additional complexity. Have a go at it if you want!

aslakhellesoy and others added 4 commits May 15, 2021 18:07
* origin/master: (50 commits)
  Update dependency @babel/core to v7.14.3
  Use v0.4.4 of build container
  Update typescript-eslint monorepo to v4.24.0
  Post release json-to-messages v8.0.0
  Release json-to-messages v8.0.0
  Release fake-cucumber v12.0.1
  Release gherkin-utils v5.0.2
  Release gherkin-streams v2.0.2
  Release message-streams v2.0.0
  Release html-formatter v14.0.0
  Release react v14.0.0
  Release compatibility-kit v5.0.0
  Post release json-formatter v18.0.0
  Release json-formatter v18.0.0
  Release query v10.0.0
  Release fake-cucumber v12.0.0
  Post release create-meta v5.0.0
  Release create-meta v5.0.0
  Release gherkin-utils v5.0.1
  Release gherkin-streams v2.0.1
  ...

# Conflicts:
#	gherkin/dotnet/.gitignore
#	gherkin/dotnet/Gherkin/Gherkin.csproj
#	gherkin/dotnet/Makefile
#	gherkin/dotnet/default.mk
@gasparnagy
Copy link
Member Author

@aslakhellesoy @mattwynne how can it be that gherkin-java does not understand "dotnet" is that running on a different docker container?

@aslakhellesoy
Copy link
Contributor

@gasparnagy yes:

common/.circleci/config.yml

Lines 609 to 610 in fff7cc2

gherkin-java:
executor: docker-circleci-openjdk

Also see my comment here: #1534 (comment)

@gasparnagy
Copy link
Member Author

gasparnagy commented May 18, 2021

@aslakhellesoy I see. So probably this was working so far, because the parser was never outdated and therefore berp wasn't invoked. But since I introduced the .berp_restored target, that runs even in case the parser is up-to-date.

Like here: https://github.com/cucumber/common/pull/1542/files#diff-fc82761c3f102c231bc696c79cacd60ef5e64fc6452ed707682cfc4b27206ceaR18

Is there an easy way in make to say that it should only invoke the target if the other targets are outdated?

@aslakhellesoy
Copy link
Contributor

Is there an easy way in make to say that it should only invoke the target if the other targets are outdated?

Yes, by using Make the way it was meant to be used :-)

Make targets should be files, not abstract names.

Here is an example:

foo:
    echo "hello"

Running make foo will execute the target body if and only if the foo file does not exist. Since the target never actually generates the foo file, it will always run. This is not how make should be used. Let's fix it:

foo:
    echo "hello" > $@

(The $@ variable is the name of the target). When you run make foo, it will run the next time, but not the second, because foo now exists.

Let's get more advanced. The foo target now depends on the bar target:

foo: bar
    echo "hello" > $@
    car bar > $@

Create the bar file manually with touch bar. Then run make foo. It will run the 1st time but not the 2nd time, as expected.

Now, if you touch bar again, the timestamp of bar will be more recent than that of foo, so running make foo again will run the target again.

I hope that helps. I can have a look at this tomorrow if you haven't figured it out.

@gasparnagy
Copy link
Member Author

@aslakhellesoy Thx. At that level I was OK, but here I need to make sure that berp is installed... that is not a classic target, because it is only needed for another target to be established. The "make" way would be (base on my understanding):

parser.java: gherkin.berp template.razor /usr/bin/berp
  berp ...

/usr/bin/berp: 
  ...install berp

This is more or less what I have now, but this will attempt to install berp even if the parser is up-to-date (on eg. the java parallel build, where it is not possible). I tried now with some tricky $eval/$call magic, but I get a stupid error message *** recipe commences before first target. Stop., so I kind of given up. If you have time tomorrow for a a quick pairing, I would be happy to fix it together (so that I can better learn from it).

Or I just give up using .NET restore for berp and upload it as a linux binary somewhere... But I thought it is easier to maintain the versions this way...

@gasparnagy
Copy link
Member Author

gasparnagy commented May 18, 2021

@aslakhellesoy for some of the languages, the parser generation fails with the new berp version and so I need to fix those still:

  • ruby
  • python
  • perl
  • objective-c

(It only passes now because the parsers are not outdated.)

@gasparnagy gasparnagy marked this pull request as ready for review May 19, 2021 08:49
@gasparnagy gasparnagy changed the title WIP: Upgrade berp Upgrade to berp 1.3, remove dependency to mono May 19, 2021
@gasparnagy gasparnagy merged commit c531ed4 into master May 19, 2021
@gasparnagy gasparnagy deleted the upgrade_berp branch May 19, 2021 10:37
@aslakhellesoy
Copy link
Contributor

🥳

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.

2 participants