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

Jump to bottom

[bug][maven] Fix custom windows classpaths in maven plugin #7587

Merged
merged 10 commits into from
Oct 18, 2020
Merged

[bug][maven] Fix custom windows classpaths in maven plugin #7587

merged 10 commits into from
Oct 18, 2020

Conversation

jimschubert
Copy link
Member

Fixes #4208

Thanks to @DenisKnoepfle for the discussion around this bug to help reproduce the underlying issue.

  • Updates WorkflowSettings to duplicate the logic which checks for classpath files (previously, would break on Windows, or on all OS if template directory did not match structures in openapi-generator's resource directory)
  • Updates java-client.xml in Maven example to allow spot-check of templateDirectory property
  • Fixes classpath check in template manager to work on Windows for paths defined in templateDir property of a codegen config
  • Adds tests for template resource locator

The Maven plugin follows a different flow for template directory versus resource loaded templates. I'd tried reproducing the reported bug, but it seems I was only testing templateDirectory under git bash so the problems didn't arise. I've tested templateDir and templateDirectory under both Mac and Windows, both locally and in a private git repo using workflows for cross-platform checks. This should be good to go, but I'd like to have another set of eyes on it before merge.

cc @OpenAPITools/generator-core-team

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@jimschubert
[maven] Custom template in java example
7c65aec
@jimschubert
[maven] Fallback to templates using classpath rather than OS-specific…
524bd10 
… paths

Previous checks would cause logic in Windows to return early, for
built-in templates only. This reorganizes and simplifies the ordering
behavior.
@jimschubert
Match classpath check in WorkflowSettings with that in TemplateManager
5875c11
@jimschubert jimschubert added this to the 5.0.0 milestone Oct 3, 2020
@auto-labeler
Copy link

auto-labeler bot commented Oct 3, 2020

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@jimschubert
[maven] Much needed unit/integration tests
5cae18f 
This follows similar approach used in PMD and other plugins managed by
maven.

Unit tests simply verify we can load configuration as expected into the
Mojo.

Integration tests execute actual sample projects bound to the current
build's Maven plugin. This uses maven-invoker-plugin, which also allows
for specifying the maven options in invoker.properties to execute the test.
It also provides a verification framework using groovy files with the
required naming convention of "verify.groovy". This allows us to quickly
and easily check that certain files are outputted by generation, and we
may also spotcheck file contents.

templateResourcePath option is skipped on windows. I've tested back to
version 3.3.3 and this doesn't seem to have worked consistently with how
the property works on non-Windows.
@jimschubert
Copy link
Member Author

@wing328 could you check out the maven unit and integration tests in this PR? 5cae18f
These will run consistently on Windows and non-Windows builds. Do you think we should move the spot check examples from Shippable to an integration test, so we can spot check file outputs and contents as well?

@jimschubert
Copy link
Member Author

Previous commits on this branch failed, and it took me a while to figure out.

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.codehaus.groovy.reflection.CachedClass (file:/root/.m2/repository/org/codehaus/groovy/groovy-all/2.4.8/groovy-all-2.4.8.jar) to method java.lang.Object.finalize()
WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.reflection.CachedClass
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
[ERROR] The following builds failed:
[ERROR] *  custom-template/pom.xml
[ERROR] *  custom-template-resource/pom.xml
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-invoker-plugin:3.2.1:run (integration-test) on project openapi-generator-maven-plugin: 2 builds failed. See console output above for details. -> [Help 1]

maven-invoker-plugin is resolving with groovy 2.4.8 in CI, but resolves locally for me to 3.0.5. I'm able to build in Java 8 and 11 locally because of the supported groovy version. I'm not sure why there's a difference in resolution between my local machine and CI, but I've verified that explicitly setting the groovy dependency on the plugin will affect the groovy version evaluating the scripts. This should allow CI to pass.

@jimschubert
Merge branch 'master' into fix-windows-classpath-maven-plugin
e64edf5 
* master: (66 commits)
  [Typescript][Angular] Fix generated README when using apiModulePrefix (#7725)
  remove outdated scala files (#7723)
  [FEAT][TYPESCRIPT-ANGULAR] Add configurationPrefix option to allow generating unique configuration token (#7731)
  [bug] Fix FILES sort and path provider issue (#7729)
  better csharp tests (#7727)
  [go] Improve examples generation (#7576)
  Fixes #7635: typescript-inversify generator wrongly handles array type parameters (#7636)
  [Java] Fix import mapping for arrays with reference items of type string (#7182)
  [Java][Native] Support oneOf/anyOf schemas (#7263)
  [BUG][Ada] Incorrect client Ada code generated (#7719)
  add cake, sbt integration (#7713)
  Use 3.0 spec in documentations, update docs  (#7710)
  remove github.com/antihax/optional from go.sum (#7692)
  Update junit to newer version (4.13.1) (#7690)
  [Fix/Dart2] Resolve an exception with status 204 and no body. (#7647)
  [typescript-angular] pass array as a single JSON string to url query when queryParamObjectFormat=json (fix #7620) (#7649)
  Add back HttpSigningConfiguration.cs
  remove HTTPSigningConfiguration.cs
  add AnyType support to Swift generators (#7644)
  fix warning, remove trailing spaces (#7659)
  ...
@jimschubert
Put maven integration tests in separate profile
7d02ffc
@jimschubert
Copy link
Member Author

I can't get some of our CI to work with the maven-invoker-plugin. I've tested this in a triggered GitHub workflow in a private repo and verified this is fixed in Windows for Java 8 and 11.

image

Loading from resource is potentially still an issue in Windows, but I couldn't find any release version which supported Maven's templateResourcePath in Windows.

Once this is merged, I'll add the maven integration tests to the check supported versions script which runs only on master.

@jimschubert
Copy link
Member Author

And templateResourcePath only appears broken in Windows when File.separator is Windows style. I'll need to dig into that a little more separately.

@jimschubert jimschubert merged commit ee1cbf6 into master Oct 18, 2020
@jimschubert jimschubert deleted the fix-windows-classpath-maven-plugin branch October 18, 2020 16:06
@DenisKnoepfle
Copy link

It looks good now and works for Windows. I confirmed it with the 5.0.0-beta3 release.

parenko added a commit to parenko/openapi-generator that referenced this pull request May 20, 2024
@parenko
Comment not required anymore
01dd879 
Was introduced with OpenAPITools#7587 could be removed with OpenAPITools#10544
wing328 pushed a commit that referenced this pull request May 21, 2024
@parenko @allen-ball
PR for openapi-generator-maven-plugin inputSpec -- Allow jar: URLs (#…
9a35914 
…18576)

* Added support for <inputSpec/> arguments of JAR URLs.
E.g., jar:jar-specific-uri!/spec.yml.

* Resolve and search COMPILE dependencies for inputSpec resource.

* Added test cases for openapi-generator-maven-plugin:generate input
specifications:

* URLs of the form jar:jar-specific-uri!/spec.yaml

* Resources on the compilation classpath

in addition to the existing FILE test case.

* Check for inputSpecFile existence

else it is a remote URL && url is not empty

* replaced deprecated usage

* use Unix separators when on win-os

* example with jar inputSpec

* Comment not required anymore

Was introduced with #7587 could be removed with #10544

* referenced same maven version

these artifacts are referenced by same ${project.version} in https://github.com/apache/maven/blob/master/pom.xml

* updating maven dependencies to 3.9.6

---------

Co-authored-by: Allen D. Ball <ball@hcf.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Issue: Bug OpenAPI Generator Maven Plugin
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

[BUG] [MVN Plugin] - <templateDirectory> is ignored
2 participants
@jimschubert @DenisKnoepfle