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

PAYARA-2215 Splitting --addjars to let the classpath separator be decided upon OS #2073

Merged
merged 5 commits into from Oct 25, 2017
Merged

PAYARA-2215 Splitting --addjars to let the classpath separator be decided upon OS #2073

merged 5 commits into from Oct 25, 2017

Conversation

svendiedrichsen
Copy link
Contributor

under Windows uses ; otherwise : is used.

@svendiedrichsen svendiedrichsen changed the title #2070 let the classpath separator be decided upon OS Splitting --uberjar let the classpath separator be decided upon OS Oct 15, 2017
@svendiedrichsen svendiedrichsen changed the title Splitting --uberjar let the classpath separator be decided upon OS Splitting --addjars to let the classpath separator be decided upon OS Oct 15, 2017
@svendiedrichsen svendiedrichsen changed the title Splitting --addjars to let the classpath separator be decided upon OS PAYARA-2070 Splitting --addjars to let the classpath separator be decided upon OS Oct 15, 2017
@svendiedrichsen
Copy link
Contributor Author

using the File.pathSeparator property to split

@mikecroft mikecroft changed the title PAYARA-2070 Splitting --addjars to let the classpath separator be decided upon OS PAYARA-2215 Splitting --addjars to let the classpath separator be decided upon OS Oct 16, 2017
@mikecroft mikecroft added PR: CLA CLA submitted on PR by the contributor community-contribution labels Oct 16, 2017
@mikecroft
Copy link
Contributor

jenkins test please

@svendiedrichsen
Copy link
Contributor Author

To my knowledge I already signed the CLA.

@mikecroft
Copy link
Contributor

Thanks @svendiedrichsen - we have it on record 😃 The green CLA label is a green light to say all is well! We have a yellow "awaiting CLA" label if we need one.

@svendiedrichsen
Copy link
Contributor Author

Thanks @mikecroft. I didn't know that.

@payara-ci
Copy link
Contributor

Quick build and test passed!

Cousjava
Cousjava approved these changes Oct 16, 2017
View reviewed changes
Copy link
Contributor

@Cousjava Cousjava left a comment

Choose a reason for hiding this comment

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

Tested and works on Linux

@svendiedrichsen
Copy link
Contributor Author

svendiedrichsen commented Oct 16, 2017 •
edited

Do we need an approval on windows too? And how about a backport to Payara 4.174?

@svendiedrichsen
Copy link
Contributor Author

svendiedrichsen commented Oct 16, 2017 •
edited

How about the fish.payara.micro.cmd.options.SeperatedFilesValidator class? It also uses : hard coded. It looks to me as if this fix won't work on windows without this class being fixed too.

@arjantijms
Copy link
Contributor

@Cousjava

Tested and works on Linux

Would probably be a good idea indeed to test this on Windows ;)

@svendiedrichsen
Copy link
Contributor Author

jenkins test please

1 similar comment
@Cousjava
Copy link
Contributor

jenkins test please

@payara-ci
Copy link
Contributor

Quick build and test passed!

@Pandrex247
Copy link
Member

Does indeed need testing on Windows before merging

@svendiedrichsen
Copy link
Contributor Author

I'd love to help out but

  • I don't have a Windows machine
  • I made the changes

@Pandrex247
Copy link
Member

@svendiedrichsen That was a poke at us not you! It had been a week since our last update on this PR, so I put the "Action Required" label and my comment as a reminder / big lilac flag to get this tested.
We're capable of testing it ourselves, we just need to actually do it and not let this languish and be forgotten about.

@Pandrex247
Copy link
Member

Fixes #2070

@arjantijms arjantijms added this to the Payara 4.174 milestone Oct 24, 2017
@MeroRai
Copy link
Member

MeroRai commented Oct 25, 2017

Tested and works in windows 10

@arjantijms
Copy link
Contributor

@MeroRai tested on Windows 10 and confirmed this to be working!

I'll now merge it in.

Thanks to everyone involved!

@arjantijms arjantijms merged commit 8763b66 into payara:master Oct 25, 2017
@smillidge smillidge mentioned this pull request Nov 5, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cousjava Cousjava Cousjava approved these changes

Assignees
No one assigned
Labels
PR: CLA CLA submitted on PR by the contributor
Projects
None yet
Milestone
Payara 4.174
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@svendiedrichsen @mikecroft @payara-ci @arjantijms @Cousjava @Pandrex247 @MeroRai