Community
Participate
Working Groups
Remote Tools should test that Java is installed and that it has the minimum required version. This should be done before trying to start a JVM for a RM or Dstore. Currently one gets an "Exit Code 127" message, on a computer which doesn't have Java installed when Dstore or the PBS RM is started. This error message is confusing. If the Java version is too old, it one also gets odd error behavior.
The server launcher supports other launch commands, not just java, so this functionality would need to be generic enough to work with any command.
(In reply to comment #1) > The server launcher supports other launch commands, not just java, so this > functionality would need to be generic enough to work with any command. I suggest to add to the extension point org.eclipse.ptp.remote.core.remoteServer versionCommand, versionRegEx and requiredVersion For java this would be versionCommand=java -version versionRegEx=java version "(.*)" requiredVersion=1.5 For python this would be versionCommand=python -V versionRegEx=Python (.*) requiredVersion=2.4 The code would run the version command, take group1 from the regex and compare this to the required version. Then if the exit code is not 0, the regex doesn't match or the version is smaller than the required version an error can be shown to the user.
I'd suggest using something more generic, e.g.: verifyCommand=... verifyPattern=... verifyResult=... so it could be used to check anything, no just version.
(In reply to comment #3) > I'd suggest using something more generic, e.g.: > > verifyCommand=... > verifyPattern=... > verifyResult=... > > so it could be used to check anything, no just version. (In reply to comment #3) > I'd suggest using something more generic, e.g.: > > verifyCommand=... > verifyPattern=... > verifyResult=... > > so it could be used to check anything, no just version. I agree. But then I don't see a reason to have verifyResult. I would suggest that that the code just checks whether verifyPattern matches the output. Then only the two options are required. The reason I originally proposed to have the pattern/regEx + the requiredVersion is that it is a little bit cumbersome to match numbers with RegEx. E.g. requiring Java 1.5 would be <<java version "(1\.[5-9]|[2-9]\.).*">> since regex don't support number comparison and thus no >1.5 is understood and thus the alternative syntax is required. But to make it more general just one (little bit more complex) is probably the best way to do it.
Created attachment 173905 [details] Fix to test Java installed on remote server This attachment contains the fix to test Java installed on remote server. The affected files are as follows; Package org.eclipse.ptp.remote.launch.core 1. AbstractRemoteServerRunner 2. RemoteServerManager Package org.eclipse.ptp.remote.core.messages 1. Messages 2. Messages.properties Package org.eclipse.ptp.rms.pbs.jproxy 1. Plugin.xml This code introduces two new methods, “isValidVersionInstalled” and “runVerifyCommand” in “AbstractRemoteServerRunner”. The purpose of these methods is to check whether the valid java version is installed on the remote server or not. (The code is quite flexible; it is not locked to “java” and can check any software, just need to provide the command in “plugin.xml” file.) Three new attributes have been added to the extension “org.eclipse.ptp.remote.core.remoteServer”. They are as follows; verifyCommand - Specify the command to check the desired software on remote server verifyPattern - Specify the search patter in form of Regular expression. verifyReqVersion – Specify the minimum required version. This attribute is used for showing error message to the user.
This patch needs some small improvements (including API tooling) before it can commited. See PM for details.
Created attachment 173947 [details] New version of patch I, Mayur Ramgir, declare that: 1) This code contains no cryptography. 2) The code has been developed from scratch (i.e. without incorporating content from elsewhere or relying on the intellectual property of others). 3) All code in the contribution is licensed under the EPL.
Applied to HEAD. (without reorder of members has less than 250 new lines and thus needs no IP review)
Please mark the patch with the iplog flag for third party contributions. Also, I thought from the bug comments that we didn't need the result, only the pattern? If it's going to be kept, then I would prefer the attribute to be called 'verifyResult' rather than 'verifyReqVersion' to make it more generic (and the documentation updated). Perhaps make this an optional attribute so that it doesn't need to be specified?
In fact, all these attributes should be optional.
(In reply to comment #9) > Also, I thought from the bug comments that we didn't need the result, only the > pattern? If it's going to be kept, then I would prefer the attribute to be > called 'verifyResult' rather than 'verifyReqVersion' to make it more generic > (and the documentation updated). Mayur pointed out to me that we should show a message to the user if the test fails. The verifyReqVersion is only used for that message - not to test the result. Currently the Message shown to the user if the test fails is "Minimum {0} is required" with {0} being the verifyReqVersion which is for the current server "Java 1.5". Should we call it verifyRequirement? (In reply to comment #10) > In fact, all these attributes should be optional. Mayur could you please change this and attach a patch for this?
I'd suggest "verifyFailMessage". Thanks.
(In reply to comment #12) > I'd suggest "verifyFailMessage". Thanks. Mayur Ramgir: Also, if I make them optional and developer only provides value for two attributes "verifyCommand" and "verifyPattern" then the system will show the following message Minimum "" is required Roland: I see two options: 1) You can make the message something like "Cannot run server because of missing requirements" if verifyFailMessage is not given 2) You only allow all or none of the verify* options to be given (you don't run the verifyCommand if one is missing and print error) I don't care which option you choose.
Created attachment 174120 [details] Updated version of patch This patch contains the following changes suggested by Greg and Roland. 1) Renamed "verifyReqVersion" to "verifyFailMessage" 2) Changed "verifyCommand", "verifyPattern" and "verifyFailMessage" attributes' use setting to optional 3) If verifyFailMessage is not given then the system will show "Cannot run server because of missing requirements" message
(In reply to comment #14) > Created an attachment (id=174120) [details] > Updated version of patch > > This patch contains the following changes suggested by Greg and Roland. Thanks. Could you please create the patch relative to the current CVS HEAD? Thus only the changes relative to the last commit. (Just update/merge your workspace from CVS and afterwards your patch should be relative to current CVS).
There's a further complication that the message needs to be translatable. This is done in the plugin manifest using "%name" to indicate that the message "name" is located in the plugin.properties file. I suggest we use a similar approach.
(In reply to comment #16) > There's a further complication that the message needs to be translatable. This > is done in the plugin manifest using "%name" to indicate that the message > "name" is located in the plugin.properties file. I suggest we use a similar > approach. The message is currently already in the message.propoerties file. The messages are: AbstractRemoteServerRunner_12= Minimum "{0}" is required AbstractRemoteServerRunner_14=Cannot run server because of missing requirements In the xml file it has only: verifyFailMessage="Java 1.5" This doesn't need translation. Thus I think it is OK.
I don't think this is general enough. The verifyFailMessage should be able to be any string the user requires. E.g. they may be testing for the presence of a command or file, not the version of java. The "AbstractRemoteServerRunner_14" string is fine for the case where no fail message is provided. Thanks!
(In reply to comment #18) > I don't think this is general enough. The verifyFailMessage should be able to > be any string the user requires. E.g. they may be testing for the presence of a > command or file, not the version of java. How about I don't use "AbstractRemoteServerRunner_12" at all and directly display "verifyFailMessage". In this case, user can provide any message they want. For example, "Minimum Java 1.5 is required". Let me know if this ok and I will make the relevant change.
(In reply to comment #19) > (In reply to comment #18) > > I don't think this is general enough. The verifyFailMessage should be able to > > be any string the user requires. E.g. they may be testing for the presence of a > > command or file, not the version of java. > > How about I don't use "AbstractRemoteServerRunner_12" at all and directly > display "verifyFailMessage". In this case, user can provide any message they > want. For example, "Minimum Java 1.5 is required". > > Let me know if this ok and I will make the relevant change. As Greg said, it should be translatable. Thus verifyFailMessage should be %PBSProxyServer.verifyFailMessage and this should be then set in plugin.properties.
I think for simplicity, it's probably ok not to worry about the translation for now.
(In reply to comment #21) > I think for simplicity, it's probably ok not to worry about the translation for > now. Does it mean that I don't do any further changes to the code other than mentioned in the comment 19?
(In reply to comment #22) > (In reply to comment #21) > > I think for simplicity, it's probably ok not to worry about the translation for > > now. > > Does it mean that I don't do any further changes to the code other than > mentioned in the comment 19? Yes. And please make the patch relative to current CVS. Thanks!
Created attachment 174268 [details] Update version 2 of patch Changes made to display "verifyFailMessage" directly to the user in case it is provided by the user. The patch is relative to current CVS.
(In reply to comment #24) > Created an attachment (id=174268) [details] > Update version 2 of patch > > Changes made to display "verifyFailMessage" directly to the user in case it is > provided by the user. The patch is relative to current CVS. Two things: - You made verifyCmd optional but you don't check that it isn't null before you run it. Please check that it isn't null and otherwise don't run it. - I noticed that you call performStringSubstitution on the verifyCommand. This is not necessary (it doesn't have any variables).
Created attachment 174299 [details] Update version 3 of patch New code has been added to check if the verify command is provided. If so then run the verification code. Also, I have removed the unnecessary string substitution call "performStringSubstitution" on the verifyCommand.
(In reply to comment #25) > (In reply to comment #24) > > Created an attachment (id=174268) [details] [details] > > Update version 2 of patch > > > > Changes made to display "verifyFailMessage" directly to the user in case it is > > provided by the user. The patch is relative to current CVS. > > Two things: > - You made verifyCmd optional but you don't check that it isn't null before you > run it. Please check that it isn't null and otherwise don't run it. I thought about it earlier but my testing showed that the code doesn't return any value if the command is not provided and hence it fulfills our criteria. However, I totally agree with you on adding a check on verfiyCommand. This way we can avoid unnecessary processing over the wire. > - I noticed that you call performStringSubstitution on the verifyCommand. This > is not necessary (it doesn't have any variables).
committed to HEAD
Created attachment 178577 [details] move test to startServer It seems that this test is being done after the server process exits. Was that intended? I would have thought the test should be done in startServer. The attached patch does this. Let me know if you're ok with this and I'll commit to head.
(In reply to comment #29) > Created an attachment (id=178577) [details] > move test to startServer > > It seems that this test is being done after the server process exits. Was that > intended? > > I would have thought the test should be done in startServer. The attached patch > does this. Let me know if you're ok with this and I'll commit to head. Our idea was to only use the verifyCommand to find out why starting the server didn't work. This has the tiny advantage that a successful server start is little bit faster because the verifyComannd doesn't has to be run. But it is also OK to move it as you did. Thus go ahead and commit it.
Committed.
