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

Update HttpRequest.cpp.mustache - use stable 4-parameter connect #18893

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

Conversation

Jazzco
Copy link

@Jazzco Jazzco commented Jun 10, 2024

The 3-parameter connect is known to cause issues when the creating instance is deleted. In all other mustache you already use stable 4-parameter connect. In HttpRequest.cpp.mustache the fix was missing.

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.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    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*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

Use stable 4-parameter connect
@wing328
Copy link
Member

wing328 commented Jun 11, 2024

thanks for the PR. please follow step 3 to update the samples.

cc @ravinikam (2017/07) @stkrwork (2017/07) @etherealjoy (2018/02) @MartinDelille (2018/03) @muttleyxd (2019/08)

Copy link
Contributor

@muttleyxd muttleyxd left a comment

Choose a reason for hiding this comment

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

Looks good, please do the step 3 though

@Jazzco
Copy link
Author

Jazzco commented Jun 11, 2024

I ran the parts of step 3, committed and pushed. Put I don't see a hint that these changes came in. Any hint what might went wrong?

@Jazzco
Copy link
Author

Jazzco commented Jun 11, 2024

Ah wait - I pushed to my master .... need to push to patch-1

@Jazzco
Copy link
Author

Jazzco commented Jun 12, 2024

Reviewing the changes I see a lot of path separator changes, obviously because I ran mvn and the scripts on Windows. Is that really what you want?

Copy link
Contributor

@MartinDelille MartinDelille left a comment

Choose a reason for hiding this comment

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

Yes please push only the change regarding HttpRequest please!

@wing328
Copy link
Member

wing328 commented Jun 12, 2024

@muttleyxd @MartinDelille when you guys have time, can you please PM me via Slack? Thanks.

@Jazzco
Copy link
Author

Jazzco commented Jun 13, 2024

Yes please push only the change regarding HttpRequest please!

As far as I see there is nothing changed but the manual change in HttpRequest, as long as the shifted entries of ApiException.php and readme.MD are irrelevant.

From a Windows user perspective the script I was asked to follow in step 3 is unusable in this state. Maybe it should be extended to replace all CR/LF by single LF.

I don't have the time to dig into it but the git bash seems to have commands for this:

find . -name "*.java" -exec dos2unix {} \;

@wing328
Copy link
Member

wing328 commented Jun 15, 2024

@Jazzco tests with updated samples failed: https://github.com/OpenAPITools/openapi-generator/actions/runs/9527512386/job/26264196606?pr=18939

can you please review the test failure when you've time?

@wing328 wing328 mentioned this pull request Jun 15, 2024
5 tasks
@Jazzco
Copy link
Author

Jazzco commented Jun 17, 2024

@Jazzco tests with updated samples failed: https://github.com/OpenAPITools/openapi-generator/actions/runs/9527512386/job/26264196606?pr=18939

can you please review the test failure when you've time?

@wing328 I reviewed the change and don't see what could cause the issue. As mentioned above, step 3 produces many line break changes when processed on Windows, so I reverted it. Maybe I oversaw a generated change that was necessary but I guess it makes way more sense to run this script on Linux in the current state.

Locally generating our api and patching the results with these two changes afterwards works fine.

@ravinikam (2017/07) @stkrwork (2017/07) @etherealjoy (2018/02) @MartinDelille (2018/03) @muttleyxd (2019/08)
Could one of you please take over this tiny change? And maybe trigger your scripting team to fix the linefeed-issue

Regards
Jazzco

@MartinDelille
Copy link
Contributor

@muttleyxd @MartinDelille when you guys have time, can you please PM me via Slack? Thanks.

I sent you an email!

…tion (see warning [clazy-connect-3arg-lambda])
@Jazzco
Copy link
Author

Jazzco commented Jun 18, 2024

I noticed that there were some other [clazy-connect-3arg-lambda] warnings, this time in api-body, and fixed them, too.

@Jazzco
Copy link
Author

Jazzco commented Jun 18, 2024

To get this right I ran step 3 from a Linux vm and pushed the generated changes now.

@Jazzco
Copy link
Author

Jazzco commented Jun 18, 2024

I tested these changes using Qt 5.15.2 on Windows. There were no compilation or link errors.

@Jazzco
Copy link
Author

Jazzco commented Jun 18, 2024

There were () missing and it obviously worked with the 3 param version but failed with the 4 param version under macOS. Hopefully it builds now.

Copy link
Contributor

@MartinDelille MartinDelille left a comment

Choose a reason for hiding this comment

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

Thank you so much for your contribution!

@wing328
Copy link
Member

wing328 commented Jun 19, 2024

the CI tests failed: https://github.com/OpenAPITools/openapi-generator/actions/runs/9567139672/job/26402928612?pr=18893

please take a look when you've time.

@Jazzco
Copy link
Author

Jazzco commented Jun 19, 2024

I'm out of ideas. The linker error is way to universal to track it down to a cause.

clang: error: linker command failed with exit code 1 (use -v to see invocation)

Maybe if you were able to activate the -v option...

@wing328
Copy link
Member

wing328 commented Jun 19, 2024

Maybe if you were able to activate the -v option...

please update the workflow file to activate that

.github/workflows/samples-cpp-qt-client.yaml 

@MartinDelille
Copy link
Contributor

I tried to reproduce the issue locally on my mac using Qt 5.15.17 without issue.

@Jazzco Jazzco requested a review from jimschubert as a code owner June 20, 2024 14:35
Copy link
Contributor

@MartinDelille MartinDelille left a comment

Choose a reason for hiding this comment

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

I guess the ci didn't ran properly because none of the cpp qt code changed. This minor edit should trigger it as normal (hopping I didn't failed suggesting it)

@Jazzco
Copy link
Author

Jazzco commented Jun 27, 2024

@MartinDelille The suggestions compile locally so I committed them.

@MartinDelille
Copy link
Contributor

@wing328 any idea why the CI doesn't trigger properly ?

@wing328
Copy link
Member

wing328 commented Jun 28, 2024

triggered the workflow but some tests failed. please review when you guys have time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants