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

[SUREFIRE-1994] Upgrade javacc-maven-plugin, add setup instructions for Eclipse #453

Merged
merged 1 commit into from Feb 4, 2022
Merged

[SUREFIRE-1994] Upgrade javacc-maven-plugin, add setup instructions for Eclipse #453

merged 1 commit into from Feb 4, 2022

Conversation

spannm
Copy link
Contributor

@spannm spannm commented Jan 29, 2022

This PR addresses the issue in [SUREFIRE-1994] to upgrade/configure javacc-maven-plugin.

✅ Each commit in the pull request should have a meaningful subject line and body.
✅ Format the pull request title like [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles
✅ Run mvn clean install to make sure basic checks pass.
(I use mvn clean verify usually which is the same but does not put artifacts into the local repo.)

✅ I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

<executions>
<execution>
<id>javacc</id>
<goals>
<goal>javacc</goal>
</goals>
<configuration>
<sourceDirectory>${project.basedir}/src/main/javacc</sourceDirectory>
<outputDirectory>${generated.sources.dir}</outputDirectory>
Copy link
Member

Choose a reason for hiding this comment

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

why change default value .... ${project.build.directory}/generated-sources/javacc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bad default imho. People don't expect source folders to exist in the build directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sman-81
Don;t modify default settings unless it is absolutely necessary.
The old plugin is compliant with IDEs, the default layout, where generates sources appear in target/generated-sources/javacc.

<executions>
<execution>
<id>javacc</id>
<goals>
<goal>javacc</goal>
</goals>
<configuration>
<sourceDirectory>${project.basedir}/src/main/javacc</sourceDirectory>
Copy link
Member

Choose a reason for hiding this comment

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

default value is ${basedir}/src/main/javacc, so looks the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like things to be obvious and explicit. Devs not familiar with the workings of this plugin will be able to understand better.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sman-81
Don;t modify default settings unless it is absolutely necessary.
The old plugin is compliant with IDEs, the default layout, where generates sources appear in target/generated-sources/javacc.
The src/main files are a subject to git commit them. So I disagree to touch this folder.

Copy link
Contributor

@Tibor17 Tibor17 Jan 29, 2022

Choose a reason for hiding this comment

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

Nowadays the IDEs can be configured to run the compilation and lookup target/generated-sources which is default path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The src/main files are a subject to git commit them. So I disagree to touch this folder.

@Tibor17
Did you notice .gitignore was updated to prevent this?

Take a look at my comment in the Jira-Ticket.

@Tibor17
Copy link
Contributor

Tibor17 commented Jan 30, 2022

I have checked it out and the following plugin fails with an error
Execution javacc of goal org.javacc.plugin:javacc-maven-plugin:3.0.3:javacc failed: A required class was missing while executing org.javacc.plugin:javacc-maven-plugin:3.0.3:javacc: org/javacc/parser/Main

              <groupId>org.javacc.plugin</groupId>
              <artifactId>javacc-maven-plugin</artifactId>
              <version>3.0.3</version>

I have used another plugin which release was made 20 days ago and it is stable

          <groupId>com.helger.maven</groupId>
          <artifactId>ph-javacc-maven-plugin</artifactId>
          <version>4.1.4</version>

The directory structure is the same and I did not provide any additional configuration (only used groupId:artifactId:version).
Used two goals for JavaDoc as well:

<goal>javacc</goal>
<goal>jjdoc</goal>

and compiled the project mvn clean install -DskipTests successfully.

@spannm
Copy link
Contributor Author

spannm commented Jan 30, 2022

@Tibor17 for org.javacc.plugin:javacc-maven-plugin you need to configure javacc as a dependency. It's in my pull request.

@Tibor17
Copy link
Contributor

Tibor17 commented Jan 30, 2022

This works as well and it is binary compliant with the old plugin.

      <plugin>
          <groupId>org.javacc.plugin</groupId>
          <artifactId>javacc-maven-plugin</artifactId>
          <version>3.0.3</version>
        <executions>
          <execution>
            <id>javacc</id>
            <goals>
              <goal>javacc</goal>
                <goal>jjdoc</goal>
            </goals>
          </execution>
        </executions>
          <dependencies>
              <dependency>
                  <groupId>net.java.dev.javacc</groupId>
                  <artifactId>javacc</artifactId>
                  <version>7.0.10</version>
              </dependency>
          </dependencies>
      </plugin>

@spannm
Copy link
Contributor Author

spannm commented Jan 31, 2022

Hello @Tibor17, @slawekjaranowski,
Updated the pr to use defaults in javacc-maven-plugin, removed build-helper-maven-plugin, add added setup instructions for development in Eclipse.

@Tibor17
Copy link
Contributor

Tibor17 commented Feb 3, 2022

@sman-81
It looks fine so far but I would like to review only one commit. Can you pls squash both commits? Thx
BTW, how is your Eclipse settings now? Did my links help in Jira?

@spannm spannm changed the title [SUREFIRE-1994] Upgrade and configure javacc-maven-plugin, add src/generated/java to .gitignore [SUREFIRE-1994] Upgrade javacc-maven-plugin, add setup instructions for Eclipse Feb 3, 2022
@spannm
Copy link
Contributor Author

spannm commented Feb 3, 2022

@Tibor17

I've merged upstream modifications, git-squashed the two commits of this ticket, and renamed the PR.

surefire-grouper/pom.xml Outdated Show resolved Hide resolved
@Tibor17
Copy link
Contributor

Tibor17 commented Feb 3, 2022

@sman-81
You used git rebase with forced push or you have used git merge?
It looks like you use merge and that's the reason why we can see commits of master as yours.
It would be maybe easier for you cherry pick only your commit and use it in a fresh PR. I am afraid this has conflicts with master since the GH does not let be merge this PR - the button is shaded.

@spannm
Copy link
Contributor Author

spannm commented Feb 3, 2022

@sman-81 You used git rebase with forced push or you have used git merge?

  1. I fetched upstream at github on my branch
  2. then pulled
  3. squashed using git rebase -i HEAD~15
  4. followed by push --force

No 1 and 2 weren't so clever I can see now.

I'll make a new pull request, ok?

@Tibor17
Copy link
Contributor

Tibor17 commented Feb 3, 2022

@sman-81
yes, feel free to go for it. Thx

@spannm
Copy link
Contributor Author

spannm commented Feb 4, 2022

Hello @Tibor17

now the pr is correctly rebased and squashed. Please take a look :)
Thanks!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Tibor17
Copy link
Contributor

Tibor17 commented Feb 4, 2022

@sman-81
@slawekjaranowski
LGTM. No objections?

@spannm
Copy link
Contributor Author

spannm commented Feb 4, 2022

@sman-81 @slawekjaranowski LGTM. No objections?

well ..., I don't :)
have a good week-end!

@Tibor17 Tibor17 merged commit 463fb64 into apache:master Feb 4, 2022
@Tibor17
Copy link
Contributor

Tibor17 commented Feb 4, 2022

Thx for contributing and the PR! Let's keep tehm coming more.

@spannm
Copy link
Contributor Author

spannm commented Feb 5, 2022

@Tibor17 Thank you, too. I like your attention to detail. It was good working together!

@spannm spannm deleted the sman-81-SUREFIRE-1994 branch February 16, 2022 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants