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

fix(#758): add path.join to build os-dependent java binary path #759

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

mok-liee
Copy link
Contributor

@mok-liee mok-liee commented Mar 5, 2024

fixes #758

@mok-liee mok-liee marked this pull request as draft March 5, 2024 17:09
@mok-liee mok-liee marked this pull request as ready for review March 5, 2024 19:17
.filter(isString)
.join(' ');

return this.isWin() ? `"${command}"` : command;
Copy link
Member

Choose a reason for hiding this comment

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

what if command contains " ?

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 generated commands already have " inside and only run successfully with these quotes in the windows build and without it will not.

concurrently expects it in this case if there are spaces in the paths, which is very common in windows. therefore i would generalize this.

i was also been able to reproduce the behavior with concurrently alone.

i can send later some examples.

the most important part of the windows build plan already looks promising. there is only one error in the last step that i can't get locally.

Error: spawn ENAMETOOLONG

sounds really special to me. i think the command is too long. i try to figure out why this is. i mean foo is not longer then bar 😂

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 got it. i removed the JAVA_HOME env wrong. for now all tests are passed.

can you tell me how i can resolve the conflicts? why does this not behave like the first PR, where i was able to resolve the conflicts?

Copy link
Member

Choose a reason for hiding this comment

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

you will need to cherry-pick 3e949d8, which was reverted by me last week.

Copy link
Member

Choose a reason for hiding this comment

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

the generated commands already have " inside and only run successfully with these quotes in the windows build and without it will not.

did you test with something like --additional-properties option="test-double-quote" ?

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 tried to cherry-pick but after resolving the conflicts there was nothing to commit. i commited with git commit --allow-empty but it will not resolve the conflicts.

did you test with something like --additional-properties option="test-double-quote" ?

i try to implement something like this to the test.

on my local env (windows - git bash) the following succeeded:

$ npm run oa generate -- -g ruby -i https://raw.githubusercontent.com/OpenAPITools/openapi-generator/master/modules/openapi-generator/src/test/resources/3_0/petstore.yaml -o ./out/ruby --additional-properties option="test-double-quote"

> [email protected] oa
> openapi-generator-cli generate -g ruby -i https://raw.githubusercontent.com/OpenAPITools/openapi-generator/master/modules/openapi-generator/src/test/resources/3_0/petstore.yaml -o ./out/ruby --additional-properties option=test-double-quote

[main] INFO  o.o.codegen.DefaultGenerator - Generating with dryRun=false
[main] INFO  o.o.codegen.DefaultGenerator - OpenAPI Generator: ruby (client)
[main] INFO  o.o.codegen.DefaultGenerator - Generator 'ruby' is considered stable.
...

$ echo $?
0

mok-liee and others added 3 commits March 6, 2024 17:40
* support JAVA_HOME environment variable

fixes  OpenAPITools#661

* test(generator-cmd): add consideration of the JAVA_HOME variable

* docs: add instructions for the use of JAVA_HOME

* fix: add quotes to accept spaces in the JAVA_HOME path

* fix: add quotation marks only for windows paths

---------

Co-authored-by: Nils Braune <[email protected]>
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.

[BUG] - Generation Error in V.2.10.0
3 participants