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

[WIP] Maven wrapper #635

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

[WIP] Maven wrapper #635

wants to merge 10 commits into from

Conversation

geofjamg
Copy link
Member

@geofjamg geofjamg commented Jul 20, 2023

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Does this PR already have an issue describing the problem?

No

What kind of change does this PR introduce?

Feature

What is the new behavior (if this is a feature change)?
We use Maven wrapper to build the Java part, so that we do not require to install a specific Maven version.

Does this PR introduce a breaking change or deprecate an API?

  • The Breaking Change or Deprecated label has been added
  • The migration guide has been updated in the github wiki (What changes might users need to make in their application due to this PR?)

Other information:

Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
README.md Show resolved Hide resolved
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
@geofjamg geofjamg changed the title [WIP] Maven wrapper Maven wrapper Jul 21, 2023
@geofjamg geofjamg requested a review from EtienneLt July 21, 2023 12:22
@EtienneLt
Copy link
Contributor

have you tested the full ci ?

@EtienneLt
Copy link
Contributor

is there anything to do with proxy configuration ?

@geofjamg
Copy link
Member Author

have you tested the full ci ?

No, but I'm confident

@geofjamg
Copy link
Member Author

is there anything to do with proxy configuration ?

@olperr1 I guess you add some doc on core side to how configure the proxy for the maven wrapper?

@olperr1
Copy link
Member

olperr1 commented Jul 24, 2023

is there anything to do with proxy configuration ?

@olperr1 I guess you add some doc on core side to how configure the proxy for the maven wrapper?

@geofjamg: I added a section about it in the README.md of powsybl-core... But we found out with @jonenst that it doesn't work totally. He just found the whole solution:

There are 2 steps:

  1. the maven wrapper distribution download
  2. the maven distribution download

For the first step, you should define the proxy in the current terminal via the http_proxy envvar.
For the second step, you should use one of the following methods:

./mvnw -DproxyHost=XXX -DproxyPort=XXX -Dhttp.proxyUser=XXX -Dhttp.proxyPassword=XXX -Djdk.http.auth.tunneling.disabledSchemes= clean

or

export MVNW_USERNAME=XXX
export MVNW_PASSWORD=XXX
./mvnw -DproxyHost=XXX -DproxyPort=XXX -Djdk.http.auth.tunneling.disabledSchemes= clean

Note that in both cases, the -Djdk.http.auth.tunneling.disabledSchemes= option should be left empty.

I will add a PR in core to amend the README.md.

@EtienneLt
Copy link
Contributor

Do we need to add the install.sh like in core ?

@geofjamg
Copy link
Member Author

@EtienneLt could you try to build with the proxy ?

@geofjamg geofjamg changed the title Maven wrapper [WIP] Maven wrapper Jul 24, 2023
@geofjamg
Copy link
Member Author

Do we need to add the install.sh like in core ?

No

@geofjamg
Copy link
Member Author

./mvnw -DproxyHost=XXX -DproxyPort=XXX -Dhttp.proxyUser=XXX -Dhttp.proxyPassword=XXX -Djdk.http.auth.tunneling.disabledSchemes= clean

This one is problematic for us, because call of mvnw is done automatically from cmake file. It means that we need to automatically define -DproxyHost=XXX -DproxyPort=XXX -Dhttp.proxyUser=XXX -Dhttp.proxyPassword=XXX from env variables int he CmakeList.txt

Or not use the Maven wrapper...

@EtienneLt
Copy link
Contributor

well I make the release if we keep this pr it will be in next release is it alright ?

@olperr1
Copy link
Member

olperr1 commented Jul 24, 2023

./mvnw -DproxyHost=XXX -DproxyPort=XXX -Dhttp.proxyUser=XXX -Dhttp.proxyPassword=XXX -Djdk.http.auth.tunneling.disabledSchemes= clean

This one is problematic for us, because call of mvnw is done automatically from cmake file. It means that we need to automatically define -DproxyHost=XXX -DproxyPort=XXX -Dhttp.proxyUser=XXX -Dhttp.proxyPassword=XXX from env variables int he CmakeList.txt

Or not use the Maven wrapper...

There is no need to execute this command every time. It can be called manually once to download the wrapper and Maven distributions. Then mvnw won't try to download anything until the maven or the maven wrapper version changes.
(Note that Maven will download artifacts. So you may have to define a proper maven settings.xml file... which can contain a proxy definition.)

@geofjamg
Copy link
Member Author

well I make the release if we keep this pr it will be in next release is it alright ?

yes, let's take time to think about the best solution for this PR

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

3 participants