Skip to content

Be more careful when using single quotes for Tcl syntax#2847

Merged
boegel merged 10 commits intoeasybuilders:developfrom
ocaisa:tcl_quoting
Apr 30, 2019
Merged

Be more careful when using single quotes for Tcl syntax#2847
boegel merged 10 commits intoeasybuilders:developfrom
ocaisa:tcl_quoting

Conversation

@ocaisa
Copy link
Copy Markdown
Member

@ocaisa ocaisa commented Apr 29, 2019 •
edited
Loading

Fixes #2845

@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Apr 29, 2019 •
edited
Loading

I'm nervous about this change, using brackets changes the Tcl treatment of strings: "When quoting with braces, no substitutions are performed.". This means you couldn't set an additional envvar that referenced $HOME if " is also present in the envvar value.

@ocaisa ocaisa mentioned this pull request Apr 29, 2019
Closed
@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Apr 29, 2019

I've gone for just escaping the double quotes in the value rather than introduce behaviour changing brackets.

@boegel boegel added the bug fix label Apr 29, 2019
@boegel boegel added this to the next release (3.9.1) milestone Apr 29, 2019
boegel
boegel requested changes Apr 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

@ocaisa Can we also add the test case mentioned in #2845 in the tests somewhere, to ensure i) the reported bug is fixed, ii) stays fixed?

I'd come up with a new (toy?) test that uses something like:

modextravars = {'TEST_FLAGS': '-Xflags1="foo bar" -Xflags2="more flags" '}

And then check whether the generated module can be loaded, and what the value is for $TEST_FLAGS?

If you need help with that, shout...

Comment thread easybuild/tools/utilities.py Outdated
@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Apr 29, 2019

@boegel Have you got an example of a test that checks if a module is load-able?

@boegel
Copy link
Copy Markdown
Member

boegel commented Apr 29, 2019

@ocaisa see test/framework/toy_build.py, there's plenty in there

@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Apr 29, 2019

@boegel So there is an interesting(?) problem with the tests. Recent versions of Lmod (7.8.5 in the tests) seem to sanitise the string so that when I compare values I get a failure:

AssertionError: '-Xflags1="foo\\ bar"\\ -Xflags2="more\\ flags"\\ ' != '-Xflags1="foo bar" -Xflags2="more flags" ':
DIFF:
- -Xflags1="foo\ bar"\ -Xflags2="more\ flags"\ ?              -     -               -       ^^
+ -Xflags1="foo bar" -Xflags2="more flags"

@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Apr 30, 2019 •
edited
Loading

There are two failing tests that occur with recent versions of Lmod. The contents of the test module are

#%Module
proc ModulesHelp { } {
    puts stderr {

Description
===========
gzip (GNU zip) is a popular data compression program as a replacement for compress


More information
================
 - Homepage: http://www.gzip.org/
    }
}

module-whatis {Description: gzip (GNU zip) is a popular data compression program as a replacement for compress}
module-whatis {Homepage: http://www.gzip.org/}

set root /tmp/eb-t6RPaR/eb-Edj_cW/eb-UQ3StE/easybuild-modgen-test-tHkXQq

conflict gzip
setenv	TEST_FLAGS		"-Xflags1=\"foo bar\" -Xflags2=\"more flags\" "

This is indeed correct so I don't really know what to do. When comparing the given value of the flag and the value after loading the module we fail with:

======================================================================
FAIL: test_tcl_quoting (__main__.TclModuleGeneratorTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/alanc/EasyBuild_Git/easybuild-framework/test/framework/module_generator.py", line 693, in test_tcl_quoting
    self.assertEqual(os.getenv(test_envvar), test_flags)
  File "/home/alanc/EasyBuild_Git/vsc-install/lib/vsc/install/testing.py", line 93, in assertEqual
    raise AssertionError("%s:\nDIFF%s:\n%s" % (msg, limit, ''.join(diff[:self.ASSERT_MAX_DIFF])))
AssertionError: '-Xflags1="foo\\ bar"\\ -Xflags2="more\\ flags"\\ ' != '-Xflags1="foo bar" -Xflags2="more flags" ':
DIFF:
- -Xflags1="foo\ bar"\ -Xflags2="more\ flags"\ ?              -     -               -       ^^
+ -Xflags1="foo bar" -Xflags2="more flags"

@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Apr 30, 2019

I checked this against Lmod 8.0.2 and the tests are passing there. Unfortunately this looks like an Lmod bug, @boegel what do I do in this case?

@boegel
Copy link
Copy Markdown
Member

boegel commented Apr 30, 2019

@ocaisa https://github.com/TACC/Lmod/blob/master/README.old#L14 suggests that the issue was fixed in Lmod 7.8.8.

Let's try and update the Lmod 7.x versions used in Travis to the latest 7.x?

We should also add an entry for latest 8.x, but let's do that in a separate PR.

And I guess we should considering dropping support for Lmod 6.x in EasyBuild 4.0?
Last Lmod 6.x release was Nov'16...

@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Apr 30, 2019

Ok, the failing tests are fixed with Lmod 7.8.8

ocaisa
ocaisa commented Apr 30, 2019
View reviewed changes
Comment thread .travis.yml
- LMOD_VERSION=6.5.1 TEST_EASYBUILD_MODULE_SYNTAX=Tcl
- LMOD_VERSION=7.8.5
- LMOD_VERSION=7.8.5 TEST_EASYBUILD_MODULE_SYNTAX=Tcl
- LMOD_VERSION=7.8.22
Copy link
Copy Markdown
Member Author

@ocaisa ocaisa Apr 30, 2019

Choose a reason for hiding this comment

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

I chose 7.8.22 rather than 7.8.90 because I saw issues on my local machine with 7.8.90 (with my Devel module that is valid with every other release)

Copy link
Copy Markdown
Member

@boegel boegel Apr 30, 2019

Choose a reason for hiding this comment

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

7.8.22 makes sense, that's the actual latest 7.8.x based on https://github.com/TACC/Lmod/blob/master/README.old#L31, 7.8.90 is more like a 8.0 beta...

@easybuilders easybuilders deleted a comment from boegelbot Apr 30, 2019
@ocaisa ocaisa requested a review from boegel April 30, 2019 10:01
@boegel
Copy link
Copy Markdown
Member

boegel commented Apr 30, 2019

Looking good, going in, thanks for your efforts on this @ocaisa!

@boegel boegel merged commit 527a439 into easybuilders:develop Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@boegel boegel boegel approved these changes

Assignees

No one assigned

Labels

bug fix

Projects

None yet

Milestone

3.9.1

Development

Successfully merging this pull request may close these issues.

2 participants

@ocaisa @boegel