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

fix Lmod spider output in generated modules #1583

Merged
merged 10 commits into from Feb 23, 2016

Conversation

pescobar
Copy link
Member

No description provided.

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2606/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2607/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2608/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@@ -244,7 +244,7 @@ def get_description(self, conflict=True):
'name': self.app.name,
'version': self.app.version,
'description': description,
'whatis_lines': '\n'.join(["module-whatis {%s}" % line for line in whatis]),
'whatis_lines': '\n'.join(["module-whatis { Description: %s}" % line for line in whatis]),
Copy link
Member

Choose a reason for hiding this comment

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

why the whitespace before Description?

@wpoely86 wpoely86 added this to the v2.7.0 milestone Feb 8, 2016
@wpoely86
Copy link
Member

wpoely86 commented Feb 8, 2016

Great catch @pescobar !

lgtm

@wpoely86
Copy link
Member

wpoely86 commented Feb 8, 2016

retest this please

@pescobar
Copy link
Member Author

pescobar commented Feb 8, 2016

@wpoely86 I was discussing this with @geimer by email (he changed this code) and this fix is not the right one

whatis section now can have different lines like this:

whatis = [
    'MVAPICH2',
    'Version: 2.1',
    'Category: library, runtime support',
    'Keywords: System, Library',
    'Description: MPI-2 implementation for Infiniband',
    'URL: http://mvapich.cse.ohio-state.edu/overview/mvapich2/',
]

with this patch if you add multiple whatis lines each of them would have a Description: string in front of it.

We need to look for a better solution. I am really interested in getting proper spider output for next easybuild release

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2643/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@wpoely86
Copy link
Member

wpoely86 commented Feb 8, 2016

@pescobar yeah, I was just looking into the source of spider to see how it works. We should definitely chance this. Do you have time to do this properly?

@pescobar
Copy link
Member Author

pescobar commented Feb 8, 2016

we must fist agree about which is a proper solution. It's not clear to me. I also asked @boegel but I think he doesn't have a clear opinion about this neither.

@wpoely86
Copy link
Member

wpoely86 commented Feb 8, 2016

Okay, we should generate a whatis section like:

module-whatis "Name: Globus-4.0"
module-whatis "Version: CTSSV 4"
module-whatis "Category: grid"
module-whatis "URL: http://software.teragrid.org/install.htm"
module-whatis "Description: Defines the TeraGrid Globus 4.0 User Environment"

We already know the name, version, url and description. For category we can use the moduleclass (maybe optional?)
The easyconfigs already have keyword whatis that works like:

whatis = ["Url: foo", "Description: bar"]

We should merge the contents of it with the default generate one (meaning, append&overwrite).

@geimer
Copy link
Contributor

geimer commented Feb 8, 2016

@wpoely86: We should give users the freedom to customize the contents of whatis to whatever they want. Different sites may have different policies w.r.t. what whatis should contain. For example, on our previous systems module whatis only showed a short one-line description of the package, while module help (which is currently filled with the contents of the description variable) showed detailed usage instructions, where to find additional information, the local contact, etc. Also, remember that Tmod only shows the last entry when executing module whatis, in which case the Description: prefix is redundant. By merging the user-provided contents with some automatically generated stuff, you prohibit customization and enforce a specific policy to everyone using EB. Moreover, how should a merge algorithm look like? What if a site only wants the one-liner specified in the easyconfig rather than the full description? How do you want to handle inconsistencies due to customization, e.g., if a site wants the URL to point to local documentation rather than the homepage? Handle the strings as key-value pairs? This seems to open a big can of worms...

To me, the culprit is that Lmod relies on a specific formatting of whatis -- following the TACC site policy. Thus I see this more as an Lmod issue than an EB issue. My current thinking is that the behavior of module spider should be configurable in Lmod (e.g., use the current behavior, print all whatis strings, extract the first paragraph of module help, ...).

@rtmclay: Any thoughts on this?

@wpoely86
Copy link
Member

wpoely86 commented Feb 8, 2016

@geimer First, does module whatis work for you? I never used it and I get tcl errors on EB modules.

What you suggest it as site customizing of everything? I don't think we can do that? I think we should do the following:

  • Add all the whatis line we can based on the information in the easyconfig (description, homepage, ...)
  • If a whatis line is present in the easyconfig, this overwrites the defaults. (so a site can add customizations per easyconfig if they wish, they can even use --amend).
  • We support a generic header and footer in which additional customization can be done.

And it Tmod has issues with multiple lines, we can make sure that description is always the last one. Or just add a config option to merge all whatis fields or not.

However, that the users gets to see when the run whatis, help or spider is up to Lmod. If you want to customize, use hooks. EB cannot do anything there.

@boegel
Copy link
Member

boegel commented Feb 8, 2016

I agree with @wpoely86 that we can probably do a better job by default. But if a whatis is specified in the easyconfig, that should indeed win, otherwise sites lose the ability to customise things (and we really need to keep the whatis stuff backward compatible now).

@pescobar: I think what you're after is just adding Description: to the line that defines description =, higher up from where you're patching things now

@wpoely86
Copy link
Member

wpoely86 commented Feb 8, 2016

@rtmclay, it is normal that spider doesn't show all whatis fields?

$ ml show gsissh/4.1
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
   /home/ward/tmp/Lmod/tcl/tacc/apps/gsissh/4.1:
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
whatis("Name: Gsissh ")
whatis("Version: 4.1 ")
whatis("Category: grid ")
whatis("URL: http://www.globus.org/toolkit ")
whatis("Description: Globus GSI OpenSSH utility ")
prepend_path("PATH","/opt/gsi-openssh-4.1/bin")
prepend_path("MANPATH","/opt/gsi-openssh-4.1/man")
prepend_path("LIBPATH","/opt/gsi-openssh-4.1/lib")
prepend_path("LD_LIBRARY_PATH","/opt/gsi-openssh-4.1/lib")
prepend_path("SHLIB_PATH","/opt/gsi-openssh-4.1/lib")
setenv("GLOBUS_LOCATION","/opt/gsi-openssh-4.1")
setenv("GLOBUS_PATH","/opt/gsi-openssh-4.1")
setenv("SASL_PATH","/opt/gsi-openssh-4.1/sasl")
setenv("RSHCOMMAND","/opt/gsi-openssh-4.1/bin/gsissh")
setenv("MYPROXY_SERVER","myproxy.teragrid.org")
setenv("GLOBUS_TCP_PORT_RANGE","50000,51000")
help([[The gsi-openssh-4.1 modulefile defines the default paths and environment
variables needed to use the Globus software and utilities
available in /share/gsi-openssh-4.1, placing them before the vendor-supplied
paths in PATH and MANPATH.:
]])

And

$ ml spider gsissh/4.1

------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  gsissh: gsissh/4.1
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
    Description:
      Globus GSI OpenSSH utility 

    This module can be loaded directly: module load gsissh/4.1

    Help:
      The gsi-openssh-4.1 modulefile defines the default paths and environment
      variables needed to use the Globus software and utilities
      available in /share/gsi-openssh-4.1, placing them before the vendor-supplied
      paths in PATH and MANPATH.:

The URL field is not shown?

@cfenoy
Copy link
Contributor

cfenoy commented Feb 22, 2016

We've hit also this issue. I understand we may want to provide more customization but we shouldn't do it if this breaks backwards compatibility.

@boegel
Copy link
Member

boegel commented Feb 22, 2016

From the looks of it, this now restores the previous behaviour of having Description: included in the default whatis line, without affecting custom whatis lines.

So, looks good to me, let's see if Jenkins agrees...

@geimer: your thoughts?

@@ -210,7 +210,7 @@ def get_description(self, conflict=True):
"""
Generate a description.
"""
description = "%s - Homepage: %s" % (self.app.cfg['description'], self.app.cfg['homepage'])
description = "Description: %s - Homepage: %s" % (self.app.cfg['description'], self.app.cfg['homepage'])

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not introduce the 'Description: ' prefix only for the whatis text (i.e., 3 lines below)? For the module help output it should be pretty clear that this is a description...

Copy link
Member

Choose a reason for hiding this comment

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

makes sense, good suggestion @geimer!

@pescobar: can you revert the change on this line, and change it below to:

whatis = ["Description: %s" % description]

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2747/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@geimer
Copy link
Contributor

geimer commented Feb 22, 2016

@boegel: I still believe that the real issue is outside of EasyBuild. But I doubt that there will be a different short-term solution -- and maybe not even a long-term one...

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2748/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2749/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2750/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

"module-whatis {foo}",
"module-whatis {bar}",
"module-whatis {Description: foo}",
"module-whatis {Description: bar}",
Copy link
Member

Choose a reason for hiding this comment

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

the Description: part is not there is whatis is specified, so change this back to what it was

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2751/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2752/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@boegel
Copy link
Member

boegel commented Feb 23, 2016

Looks good to go to me... Any last words @geimer?

@geimer
Copy link
Contributor

geimer commented Feb 23, 2016

@boegel: Looks OK. It's amazing that 6 modified lines required 10 commits ;-)

@boegel
Copy link
Member

boegel commented Feb 23, 2016

Going in, thanks for tackling this @pescobar!

boegel added a commit that referenced this pull request Feb 23, 2016
fix Lmod spider output in generated modules
@boegel boegel merged commit 96340f2 into easybuilders:develop Feb 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants