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

Task #3711 - jre of the properties file does not work under windows #3

Merged
merged 4 commits into from
Apr 4, 2019
Merged

Task #3711 - jre of the properties file does not work under windows #3

merged 4 commits into from
Apr 4, 2019

Conversation

lherschi
Copy link
Contributor

@lherschi lherschi commented Mar 22, 2019

  • Check under windows against java.exe, instead of java, both in rust-launcher and in itweb-settings
  • Unmasking the colon at properties in the rust launcher

https://icedtea.classpath.org/bugzilla/show_bug.cgi?id=3711

- Check under windows against java.exe, instead of java, both in rust-launcher and in itweb-settings
- Unmasking the colon at properties in the rust launcher
@karianna karianna added the bug Something isn't working label Mar 22, 2019
@karianna karianna requested a review from judovana March 22, 2019 12:28
File javaFile = new File(cmd + File.separator + "bin" + File.separator + "java");
File javaFile = new File(cmd + File.separator + "bin" +
File.separator + "java" +
(JNLPRuntime.isWindows() ? ".exe" : ""));
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this more spread? If so, maybe it can get its own function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry i don't understand wath you mean with "Isn't this more spread?" I am not an English native speaker.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nor am I. I suspect that java part could forget .exe on more places. So maybe it would be good to ahve proper metho to appned .exe if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All others I know are in the rust sources.

@@ -85,7 +85,7 @@ fn check_file_for_property(file: File, key: &str) -> Option<String> {
None => {}
Some(kvv) => {
if kvv.key.eq(key) {
return Some(escape_unicode(kvv.value));
return Some(escape_unicode(kvv.value.replace(r"\:", ":")));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain why it is necessary?
Will it not be soved by rust-lang/rust#27791 ? Which should be heading to stable soon.

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 colon is a delimiter for Java properties. Therefore it is masked during serialization. When used in Java getProperty, it will automatically unmask. In Rust you have to take care of the unmasking yourself. How it should help escape_unicode (), does not open to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Colon xor equals is delimiter, depends what comes first. Isnt that?

Copy link
Contributor

Choose a reason for hiding this comment

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

ping?

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 following line has itweb-settings written in my deployments.properties.

deployment.jre.dir=C\:\\Program Files\\AdoptOpenJDK\\jdk-8.0.202.08\\jre

Here is the equal sign the delimiter, but java has masked the colon anyway. Without the replace, this is an invalid path, which will simply not be accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. gave sense now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants