Bioconductor / BBS

The Bioconductor Build System
9 stars 11 forks source link

Set config = False if Windows to get Java version #333

Closed jwokaty closed 1 year ago

jwokaty commented 1 year ago

Close #332

Should be tested on devel first.

hpages commented 1 year ago

Thanks Jen for working on this.

I can see 3 problems with this PR:

  1. The name of the config variable strongly suggests that it contains the opposite of what it actually contains i.e. it's set to True when the config argument must be set to False, and vice-versa. This is very miselading! Easy way to avoid this is to negate the value before assigning it to the config variable, e.g. by doing config = not BBSutils.getNodeSpec(BBSvars.node_hostname, 'OS').find('Windows').
  2. However, note that detection of the Windows platform should be done by comparing sys.platform with 'win32'. This is how it's done everywhere else in BBS's Python code, and I think it's important to be consistent.
  3. When config is False, what needs to be supplied to write_sys_command_version() is the name of the executable. In the case of Java, the name of the executable is java (lower-case), not JAVA.

So all in all, a good old if statement should do it:

    if sys.platform == 'win32':
        write_sys_command_version('java', config=False)
    else:
        write_sys_command_version('JAVA')

Thanks again, H.

jwokaty commented 1 year ago

You should get the credit for this. I will close this and you can make the PR. Thanks.

hpages commented 1 year ago

Really? I could, but that was not my intention. I think you should still make the change, deploy it on the build machines, observe how it performs on the following report, adjust for any omissions or remaining issue, etc... Thanks again!