Fix fpm description by wrapping in shell quote by rjeschmi · Pull Request #2124 · easybuilders/easybuild-framework

rjeschmi · 2017-02-16T11:16:54Z

Still need to write the test.

rjeschmi · 2017-02-16T11:17:26Z

This will resolve #2014

boegel · 2017-02-16T11:31:15Z

        '--version', pkgver,
        '--iteration', pkgrel,
-        '--description', quote_str(easyblock.cfg["description"]),
+        '--description', quote_str(eb_shell_quote(easyblock.cfg["description"])),


@rjeschmi do we need both quote_str and eb_shell_quote?

I'll check. I thought the quote_str would handle the outside wrapping, but it probably doesn't work the way I think it does.

The tests should help me figure it out I'd think.

You can see now that I've removed both quotes. If you pass the commands as a list it will handle doing the escaping properly.

It is necessary to pass each separate part of the command line as its own list entry though (so I had to also change the way I was handling the excludes and such).

I think I'm missing a test case...

rjeschmi · 2017-02-17T01:44:00Z

well the test is failing nicely, but it seems like both escapes are not enough :)

    raise EasyBuildError('cmd "%s" exited with exitcode %s and output:\n%s', cmd, ec, stdouterr)
EasyBuildError: 'cmd "fpm --workdir /tmp/eb-fveqiq/eb-OWKZv5/eb-fh53Jr/eb-CTbwdf/eb-pkgs-WRzb40 --name "toy-0.0-gompi-1.3.12-test" --provides "toy-0.0-gompi-1.3.12-test" -t rpm -s dir --version eb-3.1.0 --iteration 1 --description "\'Toy C program. Now with `backticks\\\'\'" --url "http://hpcugent.github.com/easybuild" --exclude "tmp/eb-fveqiq/eb-OWKZv5/tmpQlV2Qs/software/toy/0.0-gompi-1.3.12-test/easybuild/*.log" --exclude "tmp/eb-fveqiq/eb-OWKZv5/tmpQlV2Qs/software/toy/0.0-gompi-1.3.12-test/easybuild/*.md"  --depends "gompi-1.3.12" /tmp/eb-fveqiq/eb-OWKZv5/tmpQlV2Qs/software/toy/0.0-gompi-1.3.12-test /tmp/eb-fveqiq/eb-OWKZv5/tmpQlV2Qs/modules/all/toy/0.0-gompi-1.3.12-test.lua" exited with exitcode 1 and output:\n/bin/bash: -c: line 0: unexpected EOF while looking for matching ``\'\n/bin/bash: -c: line 1: syntax error: unexpected end of file\n'

rjeschmi · 2017-02-26T18:58:36Z

    """
    # escape any non-escaped single quotes, and wrap entire token in single quotes
-    return "'%s'" % re.sub(r"(?<!\\)'", r"\'", str(token))
+    return "'%s'" % re.sub(r"(?<!\\)'", r"'\''", str(token))


I think this is "more correct" but I'm no longer using this command so I could remove this from this PR.

if this is not relevant to this PR, please roll back (and maybe open a new PR for it)

boegel · 2017-02-28T18:13:45Z

@rjeschmi I plan to do a v3.1.1 bugfix release this week, any idea how close this is to being good to go?

rjeschmi · 2017-02-28T19:34:48Z

I don't think it is too far. I'll maybe remove the changes to eb_shell_quote?

rjeschmi · 2017-02-28T19:35:22Z

and you wanted me to write command lists to tuples right?

boegel · 2017-02-28T19:37:06Z

    """
    # escape any non-escaped single quotes, and wrap entire token in single quotes
-    return "'%s'" % re.sub(r"(?<!\\)'", r"\'", str(token))
+    return "'%s'" % re.sub(r"(?<!\\)'", r"'\''", str(token))


if this is not relevant to this PR, please roll back (and maybe open a new PR for it)

boegel · 2017-02-28T19:37:41Z

    This function will build a package using fpm and return the directory where the packages are
    """
+    # avoid circular import, should move eb_shell_quote
+    from easybuild.tools.options import eb_shell_quote


no longer used, so please remove?

boegel · 2017-02-28T19:40:01Z

    _log.debug("The list of excluded files passed to fpm: %s", exclude_files_glob)
+    # use env to find FPM https://stackoverflow.com/questions/5658622/python-subprocess-popen-environment-path
    cmdlist = [
+        '/usr/bin/env',


this should be done by run_cmd in case shell=False is used?

possibly, but do you think shell=false will rely on path in all cases?

boegel · 2017-02-28T19:41:05Z

    if build_option('debug'):
        cmdlist.append('--debug')
-
+    cmdlist.extend(deplist)


should we move down the definition of deplist here, to keep things together?

boegel · 2017-02-28T19:41:26Z

        easyblock.module_generator.get_module_filepath(),
    ])
    cmd = ' '.join(cmdlist)
+    print "cmd: %s" % cmdlist


please remove, or add to debug log statement below

boegel · 2017-02-28T19:42:27Z

+    print "cmd: %s" % cmdlist
    _log.debug("The flattened cmdlist looks like: %s", cmd)
-    run_cmd(cmd, log_all=True, simple=True)
+    run_cmd(cmdlist, log_all=True, simple=True, cache=False, shell=False)


it's not immediately clear to me why you are disabling caching here?

the cache doesn't work (in that it tries to hash a list, I will try to fix that, but this command is also not cacheable).

boegel · 2017-02-28T19:44:31Z

+versionsuffix = '-test'
+
+homepage = 'http://hpcugent.github.com/easybuild'
+description = "Toy C program. Now with `backticks'"


rather than adding a new easyconfig, can we just use toy-0.0.eb, read it, do:

toytxt = re.sub('description = .*', 'description = "Toy C program. Now with `backticks'", toytxt)

and then write it to a temporary file to use for the test?

boegel · 2017-02-28T19:45:38Z


+        ec_desc = EasyConfig(
+            os.path.join(test_easyconfigs, 't', 'toy', 'toy-0.0-gompi-1.3.12-test-description.eb'),
+            validate=False)


do a hard check on the description with the backtick in it, to make it clear why this is an important test case

boegel · 2017-02-28T19:46:16Z

+            validate=False)
+        easyblock_desc = EB_toy(ec_desc)
+        easyblock_desc.run_all_steps(False)
+        pkgdir = package(easyblock_desc)


also check that the package is really there?

rjeschmi · 2017-03-01T13:07:28Z

I think this resolves the review and it passed package test locally.

boegel · 2017-03-01T18:49:38Z


    _log.debug("Got the PNS values name: %s version: %s release: %s", pkgname, pkgver, pkgrel)
+    cmdlist = [
+        '/usr/bin/env',


@rjeschmi I really think this belongs in run_cmd...

boegel · 2017-03-01T18:51:15Z

            _log.debug("The dep added looks like %s ", dep)
            dep_pkgname = package_naming_scheme.name(dep)
-            depstring += " --depends %s" % quote_str(dep_pkgname)
+            deplist.extend(["--depends", dep_pkgname])


why not just extend cmdlist?

also, the extend(deplist) below should be right below this for loop, having something unrelated in between is confusing (although it doesn't matter anymore if you just extend to cmdlist straight away)

It just kept it more like it was, but sure I can do it the other way now too since I have the cmdlist moved up.

boegel · 2017-03-01T18:51:44Z

-    if build_option('debug'):
-        cmdlist.append('--debug')
+    _log.debug("The list of excluded files passed to fpm: %s", exclude_files_glob)
+    # use env to find FPM https://stackoverflow.com/questions/5658622/python-subprocess-popen-environment-path


comment is out of place?

boegel · 2017-03-01T18:52:04Z

-    cmdlist.extend(exclude_files_glob)
+    exclude_files_cmd = []
+    for x in exclude_files_glob:
+        exclude_files_cmd.extend(['--exclude',  os.path.join(easyblock.installdir.lstrip(os.sep), x)])


same here, just use cmdlist.extend?

boegel · 2017-03-01T18:53:05Z

-        if cmd in cache:
-            _log.debug("Using cached value for command '%s': %s", cmd, cache[cmd])
-            return cache[cmd]
+        # fetch from cache if available, cache it if it's not, but only on cmd strings


why only on strings?

if they're a list, you can just use tuple(cmd) as cache key?

You only cache based on strings. It seems safer to harden the code for what is expected rather than trying to handle all sorts of things that aren't necessary (including lists since none of the cached commands are lists at the moment and it doesn't work).

This one not done. Rest are done.

Since we're only using strings in CACHED_COMMANDS, why do we need to check the type of cmd at all?

Since we never cache a non-string cmd right now, cmd in cache will always be False, and so we'll always call out to the actual func?

This looks like a case of premature optimisation here, especially for a command that's executed very rarely...

OK, ignore my rant, the cmd in cache will fail if cmd is of type list:

>>> [] in {} Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: unhashable type: 'list'

boegel · 2017-03-01T18:54:05Z

+    _log.info('running cmd: %s ' % cmd)
    try:
-        p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
+        p = subprocess.Popen(cmd, shell=shell, stdout=subprocess.PIPE, stderr=subprocess.STDOUT,


prefix cmd with /usr/bin/env if shell is False (and take into account that cmd may be either a string or a list/tuple)

ok, I worry that it will have side effects, but it should be backwards compatible anyway.

boegel · 2017-03-01T18:56:05Z

+        self.assertTrue(os.path.isfile(pkgfile))
+        pkgtxt = read_file(pkgfile)
+        regex_pkg_regex = re.compile(r"""DESCRIPTION:.*`backticks'.*""")
+        self.assertTrue(regex_pkg_regex.search(pkgtxt))


self.assertTrue(regex_pkg_regex.search(pkgtxt), "Pattern '%s' not found in: %s" % (regex.pattern, pkgtxt))

rjeschmi · 2017-03-02T02:23:54Z

I've handled the rest of the comments save the one about checking for string in the cache code. All of the commands in CACHED_COMMANDS are strings.

If it is really a useful feature to allow it I think you should rethink how cached commands are handled. Right now it works and is better performing since it doesn't uselessly convert the command list to a tuple to check against a constant list of strings.

boegel · 2017-03-03T12:48:27Z

 from easybuild.tools.toolchain import DUMMY_TOOLCHAIN_NAME
 from easybuild.tools.utilities import import_available_modules, quote_str
-
+from subprocess import list2cmdline


@rjeschmi unused import?

yeah sorry I was using it to debug, should have checked pylint.

I actually removed quote_str as well since I no longer use it with the cmd list version.

boegel · 2017-03-03T12:51:41Z

-        if cmd in cache:
-            _log.debug("Using cached value for command '%s': %s", cmd, cache[cmd])
-            return cache[cmd]
+        # fetch from cache if available, cache it if it's not, but only on cmd strings


Since we're only using strings in CACHED_COMMANDS, why do we need to check the type of cmd at all?

Since we never cache a non-string cmd right now, cmd in cache will always be False, and so we'll always call out to the actual func?

This looks like a case of premature optimisation here, especially for a command that's executed very rarely...

boegel · 2017-03-03T12:57:41Z

+    if not shell and isinstance(cmd, list):
+        cmd.insert(0, '/usr/bin/env')
+    elif not shell and isinstance(cmd, str):
+        cmd = '/usr/bin/env %s' % cmd


# run command via 'env' if it is not run in a shell to trigger $PATH lookup of used command; # cfr. <include-missing-reference> if not shell: if isinstance(cmd, list): cmd.insert(0, '/usr/bin/env') elif isinstance(cmd, basestring): cmd = '/usr/bin/env ' + cmd else: raise EasyBuildError("Don't know how to prefix with /usr/bin/env for commands of type %s", type(cmd))

boegel · 2017-03-03T13:06:07Z

-            return cache[cmd]
+        # fetch from cache if available, cache it if it's not, but only on cmd strings
+        if isinstance(cmd, str):
+            if cmd in cache:


let's chain these together:

if isinstance(cmd, basestring) and cmd in cache:

you want me to prefix the next conditional with isinstance as well? Just seems harder to read...

it'll avoid the indent and the extra else at the end which is just a trimmed-down version of the inner else

ok, thanks for the explanation on chat. I have it fixed now.

rjeschmi · 2017-03-03T13:51:47Z

I think I got them all this time.

boegel · 2017-03-03T15:51:45Z

        "debug": ('', 'on')[DEBUG],
-        "debug_fpm_file": os.path.join(tmpdir, DEBUG_FPM_FILE)}
-    )
+        "debug_fpm_file": os.path.join(tmpdir, DEBUG_FPM_FILE)})


put }) on new line instead, indenting like ) was before

boegel · 2017-03-03T15:54:12Z

            print read_file(pkgfile)

+        toy_txt = read_file(os.path.join(test_easyconfigs, 't', 'toy', 'toy-0.0-gompi-1.3.12-test.eb'))
+        replace_str = '''description = """Toy C program. Now with `backticks' ''' "\n" '''and newlines"""'''


since you're using triple quotes, you can just insert a hard newline, but then it looks icky due to left indent

you can also:

replace_str = '''description = """Toy C program. Now with `backticks'\n'''' replace_str += '''and newlines"""'''

this is a syntax error. I assume you mean one less '

yes, first line has an extra trailing '... I didn't say I tested this ;)

boegel · 2017-03-03T15:56:01Z

+            "Pattern '%s' found in: %s" % (desc_backticks_regex.pattern, toy_txt))
+        ec_desc = EasyConfig(
+            toy_file,
+            validate=False)


looks a bit funky here, not the usual code style

how about:

regex = re.compile(r"""`backticks'""") self.assertTrue(regex.search(toy_txt), "Pattern '%s' found in: %s" % (regex.pattern, toy_txt)) ec_desc = EasyConfig(toy_file, validate=False)

boegel · 2017-03-03T17:42:55Z

Going in, thanks for the effort @rjeschmi!

wrap desc in shell quote

828b7e2

boegel reviewed Feb 16, 2017

View reviewed changes

boegel added this to the 3.2.0 milestone Feb 16, 2017

getting testing working

7b192cb

rjeschmi commented Feb 26, 2017

View reviewed changes

rjeschmi added 2 commits February 26, 2017 14:03

return tests to non debug state

e0c2cdd

remove missed pass in undebug

53f4369

boegel modified the milestones: 3.1.1, 3.2.0 Feb 28, 2017

boegel requested changes Feb 28, 2017

View reviewed changes

rjeschmi added 4 commits March 1, 2017 06:02

rollback eb_shell_quote

f8b991d

remove circular import avoidance

9c059f0

reorder cmdlist building and remove cache option in run_cmd

a061e88

remove special easyconfig and fix asserts

3f07335

boegel requested changes Mar 1, 2017

View reviewed changes

fixing the reviewed comments

4a4d191

boegel requested changes Mar 3, 2017

View reviewed changes

fix from review

4c32ed9

rjeschmi added 2 commits March 3, 2017 08:54

accidentally chose a bad variable

45fda0f

extending test to include newlines

2244a06

boegel requested changes Mar 3, 2017

View reviewed changes

re re re review response

a948c5a

boegel approved these changes Mar 3, 2017

View reviewed changes

boegel merged commit 9fa8e54 into easybuilders:develop Mar 3, 2017

boegel mentioned this pull request Mar 7, 2017

Bash escape issues can cause fpm command to fail when --package is used #2014

Closed

Conversation

rjeschmi commented Feb 16, 2017

Uh oh!

rjeschmi commented Feb 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjeschmi commented Feb 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Feb 28, 2017

Uh oh!

rjeschmi commented Feb 28, 2017

Uh oh!

rjeschmi commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjeschmi commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjeschmi commented Mar 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

boegel Mar 3, 2017 •

edited

Loading