Miscellanous fixups of a miscellaneous nature #171

eli-schwartz · 2018-06-19T06:56:30Z

Initially motivated by the desire to update the PGP error string, then I found subprocesses using shells everywhere and jumped down the rabbit hole.

polygamma · 2018-06-19T07:00:50Z

I am glad you found the way back out of the rabbit hole! Going to review and test your changes in detail.

eli-schwartz · 2018-06-19T07:04:33Z

src/aurman/wrappers.py

-    expac_base_query = "expac {} '{}'".format(option, "?!".join(["%{}".format(formatter) for formatter in formatting]))
-    base_query_length = len(expac_base_query.encode("utf8"))
-    queries_parameters = split_query_helper(max_query_length, base_query_length, 1, targets)
+    cmd = ["expac", option, "?!".join(["%{}".format(formatter) for formatter in formatting])] + targets



I don't even understand what this used to be doing. Why does this need to be split? ARG_MAX is quite reasonable, but if not, then it should accept names on stdin.

When being run with subprocess.run, the command may be too long to be executed, I had those values researched when I coded that part, it really is needed to split the query, if it is too long.

Well, splitting it on 131071 doesn't even make sense, how do you know what ARG_MAX is.

But like I said, if you're genuinely afraid the number of targets will overflow ARG_MAX, the appropriate response is not to try splitting it into multiple commands, but switch modes of operation and tell expac to read from the not-ARG_MAX-limited stdin.

It's not about the number of arguments, but the number of characters of the query

ARG_MAX is the kernel-defined character (not argument) limit of a command line being passed through exec(2). Is there something different, which is expac-specific, that you are referring to?

Because the number in question is the current value of ARG_MAX from uapi/linux/limits.h but again this is not really a reliable way of determining what is safe, and it's probably better to skip the issue entirely.

Oh, sorry, I misinterpreted the meaning of ARG_MAX then, guess it's the value I looked up back then, when I researched the max limit. I do not see any way listed on https://github.com/falconindy/expac to make expac read from stdin however.

If you use - as a target (and /dev/stdin is not a terminal), expac will read targets from standard input.

For example (use printf so standard input is not a terminal)

$ printf "%s\n" aurman coreutils pacman aurman coreutils pacman $ printf "%s\n" aurman coreutils pacman | expac -Q "%n %v" - aurman 2.12-1 coreutils 8.29-1 pacman 5.1.0-2

Yup, like I said and, specifically, implemented in 4ac65c9

eli-schwartz · 2018-06-19T07:05:13Z

src/docker_tests/install_tests.py

    test_command("rm -rf expac-git/")

    # install aurman
-    test_command("sudo python setup.py install --optimize=1", "/home/aurman/aurman-git")
+    test_command("sudo python setup.py install --optimize=1", dir_to_execute=d(d(d(a(__file__)))))



Tests are less reliant on being run inside docker, hardcoded paths kind of suck.

eli-schwartz · 2018-06-19T07:05:31Z

src/docker_tests/install_tests.py

-        return_code = run(command, shell=True, stdout=DEVNULL, stderr=DEVNULL).returncode
-    else:
-        return_code = run(command, shell=True, stdout=DEVNULL, stderr=DEVNULL, cwd=dir_to_execute).returncode
+    test_output = run(command, shell=True, stdout=DEVNULL, stderr=PIPE, universal_newlines=True, cwd=dir_to_execute)



This whole function actually could be used a lot more generically everywhere, I stopped trying to see error output on Travis partway through, so it only touched this file. Oh well.

eli-schwartz · 2018-06-19T07:30:08Z

It often seems to claim failure on aurman -Syu cower --noedit --pgp_fetch --keyserver hkp://ipv4.pool.sks-keyservers.net:11371 --noconfirm, with keyserver issues.

Rebase with more thorough fix for error codes, which might fix the timing or else expose the error more obviously.

polygamma · 2018-06-19T07:32:56Z

Actually I had planned to not use "cower" anymore due to the needed PGP key. I had noticed that, too, and wanted to change all that when rewriting most of the docker tests. had planned to do that after my exams, which start on Monday... fun times

polygamma · 2018-06-19T07:50:47Z

src/aurman/classes.py


        # if last commit seen hash file does not exist - create
        if not os.path.isfile(last_commit_hash_file):
            empty_tree_hash = run(
-                "git hash-object -t tree --stdin < /dev/null",
-                shell=True,
+                ["git", "hash-object", "-t", "tree", "/dev/null"],


e. g. here, how do you know, that this is doing exactly the same as executing git hash-object -t tree --stdin < /dev/null

Well, running them both produces the same results, because they're both reading the contents of /dev/null. It's actually a constant in the git source code, but using hash-object is recommended because that does not depend on git using SHA1.

Okay, maybe I have not made clear, what I wanted to know. The List writing ["git", "hash-object", "-t", "tree", "/dev/null"] leaves out --stdin <, but why is still exactly that being executed?

["git", "hash-object", "-t", "tree", "/dev/null"] will execute git hash-object -t tree /dev/null

git hash-object <file> is equivalent to git hash-object --stdin < <file> (see man git-hash-object)

git hash-object -t tree /dev/null is the recommended method of deriving the empty tree object (for example, the official sample pre-commit hook)

saleemrashid · 2018-06-19T12:25:42Z

src/docker_tests/install_tests.py

    test_command("rm -rf expac-git/")

    # install aurman
-    test_command("sudo python setup.py install --optimize=1", "/home/aurman/aurman-git")
+    test_command("sudo python setup.py install --optimize=1", dir_to_execute=d(d(d(a(__file__)))))


In the previous invocation, you omit dir_to_execute. Since this invocation is in the same directory, doesn't it make sense to omit it here?

Who omitted what?

I modified an existing instance to derive the dir_to_execute from the code itself rather than making assumptions about where the testsuite is cloned to.

Ah, I was referring to the fact that the tests are executing from /home/aurman anyway. But your approach seems more robust.

saleemrashid · 2018-06-19T12:27:21Z

src/docker_tests/archive_tests.py

        print("Error with: '{}'".format(command))

-    if CurrentTest.to_return == 0 and return_code != 0:
+    if CurrentTest.to_return == 0 and test_output.returncode != 0:


Why is it necessary to test for CurrentTest.to_return == 0?

Not especially relevant to this PR, I guess.

polygamma · 2018-06-19T18:37:23Z

Thanks to @eli-schwartz for your effort and also thanks to @saleemrashid for the explanations and also the effort trying to further improve this PR. I am currently preparing for exams, hence I am not going to merge this over the next days (I guess), because I also want to look through the whole aurman sourcecode to make sure, that there is nothing left, which has not been changed accordingly. I am however going to rewrite the docker tests first, so that it is less pain to work with them.

eli-schwartz · 2018-06-19T22:49:01Z

Rebase on top of #173

polygamma · 2018-06-20T10:08:19Z

Have changed the docker tests, see: 63b126a

Currently still using cower and cower-git, do you have any suggestions for better options? Should be quite fast to build, conflict each other and do not need a PGP key.

eli-schwartz · 2018-06-20T15:03:13Z

Rebase on master, dropping the docker test changes and dropping 016ad45 since the same change got manually added in fc5d55b.

polygamma · 2018-06-20T18:40:52Z

I changed the keyserver for the docker tests, I guess using the "pool" was the problem, I took the ubuntu keyserver for the joke of it

eli-schwartz · 2018-06-28T15:00:11Z

Status?

polygamma · 2018-06-28T15:02:25Z

got my last exam next week, will look into and merge this PR probably on the weekend, so 6-8 July

eli-schwartz · 2018-06-28T17:54:04Z

Rebased to resolve conflicts with #186

polygamma · 2018-06-30T19:53:42Z

I have cherrypicked some commits, rest to be done next week

Don't fork subprocesses in order to handle things in the stdlib (python has a very decent in-process C module for this). Also DRY. Also better chained error messages.

eli-schwartz · 2018-07-01T02:14:38Z

Rebased to remove cherry-picked commits 989ce00 7984f37 2666fb7

In nearly every case it is better to run subprocess.run() without the burden of forking a bash shell too. All it takes is to turn strings into arrays of strings and *remove* some use of formatting. Aurman specifically, does not use *any* of the fancy shell interpretation that sometimes makes forking a shell to fork the commands, a worthwhile endeavor. So don't do so. This is further cleanup after the removal of subprocesses for file operations. Only changes where this cleanly applies have been done for now... in a followup commit, subprocesses that handle arguments as heavily-quoted strings will be handled.

Some functions pass around lots of string-formatted arguments. Handling this properly would require major refactoring to use lists everywhere. For now, use shlex to convert strings to a proper list-based array that subprocess.run can elegantly handle without running its own shell parser.

Instead of decoding the stdout of subprocess.run and then doing our own writes, just hand subprocess the file object and let it write directly.

Appending is a bit faster than constucting new lists on top of the old ones. It also avoids the need to use two lists.

eli-schwartz · 2018-07-01T20:55:46Z

Develop branch looks good, and has the heroic quality of making aurman actually use native lists for this, instead of the shlex dance, which is definitely nice.

Thanks!

polygamma · 2018-07-01T20:59:57Z

see: 5a40935...32e2ac6

eli-schwartz commented Jun 19, 2018

View reviewed changes

eli-schwartz force-pushed the fixups branch from 224f9a3 to 4d7aa82 Compare June 19, 2018 07:28

polygamma reviewed Jun 19, 2018

View reviewed changes

eli-schwartz force-pushed the fixups branch from 4d7aa82 to 47b6b2a Compare June 19, 2018 08:40

saleemrashid reviewed Jun 19, 2018

View reviewed changes

eli-schwartz force-pushed the fixups branch from e5501aa to c7b052d Compare June 19, 2018 17:13

polygamma mentioned this pull request Jun 19, 2018

Issue#40 - aurman -Si #172

Closed

eli-schwartz force-pushed the fixups branch from d0cb6b9 to 40ae080 Compare June 19, 2018 22:48

eli-schwartz force-pushed the fixups branch from 40ae080 to ba25d47 Compare June 20, 2018 15:03

eli-schwartz mentioned this pull request Jun 20, 2018

Can't install/update aurman: One or more PGP signatures could not be verified! #178

Closed

eli-schwartz force-pushed the fixups branch from ba25d47 to 52fd97c Compare June 20, 2018 18:10

eli-schwartz force-pushed the fixups branch 2 times, most recently from 321746f to de85983 Compare June 20, 2018 22:53

eli-schwartz force-pushed the fixups branch from de85983 to 83f42fc Compare June 28, 2018 17:53

Use real python to handle file operations.

dd4b071

Don't fork subprocesses in order to handle things in the stdlib (python has a very decent in-process C module for this). Also DRY. Also better chained error messages.

eli-schwartz force-pushed the fixups branch from 83f42fc to a963675 Compare July 1, 2018 02:11

eli-schwartz force-pushed the fixups branch from a963675 to 9b2cd5a Compare July 1, 2018 19:37

eli-schwartz added 6 commits July 1, 2018 15:57

Be DRY when checking how to call pacman

f9f703b

expac wrapper: pass targets on stdin, avoids PATH_MAX

b13eccd

Write directly to file objects when possible.

637682d

Instead of decoding the stdout of subprocess.run and then doing our own writes, just hand subprocess the file object and let it write directly.

Refactor to use slightly more efficient list construction

bb1a936

Appending is a bit faster than constucting new lists on top of the old ones. It also avoids the need to use two lists.

eli-schwartz force-pushed the fixups branch from 9b2cd5a to bb1a936 Compare July 1, 2018 20:01

polygamma added a commit that referenced this pull request Jul 1, 2018

refactor whole aurman based on the following PR by @eli-schwartz #171

b56691d

polygamma closed this Jul 1, 2018

polygamma mentioned this pull request Jul 4, 2018

aurman fails to run on Raspberry Pi 2 #200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellanous fixups of a miscellaneous nature #171

Miscellanous fixups of a miscellaneous nature #171

eli-schwartz commented Jun 19, 2018

polygamma commented Jun 19, 2018

eli-schwartz Jun 19, 2018

polygamma Jun 19, 2018

eli-schwartz Jun 19, 2018

polygamma Jun 19, 2018

eli-schwartz Jun 19, 2018

polygamma Jun 19, 2018

saleemrashid Jun 19, 2018

eli-schwartz Jun 19, 2018

eli-schwartz Jun 19, 2018

eli-schwartz Jun 19, 2018

eli-schwartz commented Jun 19, 2018

polygamma commented Jun 19, 2018 •

edited

polygamma Jun 19, 2018

eli-schwartz Jun 19, 2018

polygamma Jun 19, 2018

saleemrashid Jun 19, 2018

saleemrashid Jun 19, 2018

eli-schwartz Jun 19, 2018

saleemrashid Jun 19, 2018

saleemrashid Jun 19, 2018

eli-schwartz Jun 19, 2018

polygamma commented Jun 19, 2018

eli-schwartz commented Jun 19, 2018

polygamma commented Jun 20, 2018

eli-schwartz commented Jun 20, 2018

polygamma commented Jun 20, 2018 •

edited

eli-schwartz commented Jun 28, 2018

polygamma commented Jun 28, 2018

eli-schwartz commented Jun 28, 2018

polygamma commented Jun 30, 2018

eli-schwartz commented Jul 1, 2018

eli-schwartz commented Jul 1, 2018

polygamma commented Jul 1, 2018

Miscellanous fixups of a miscellaneous nature #171

Miscellanous fixups of a miscellaneous nature #171

Conversation

eli-schwartz commented Jun 19, 2018

polygamma commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-schwartz commented Jun 19, 2018

polygamma commented Jun 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polygamma commented Jun 19, 2018

eli-schwartz commented Jun 19, 2018

polygamma commented Jun 20, 2018

eli-schwartz commented Jun 20, 2018

polygamma commented Jun 20, 2018 • edited

eli-schwartz commented Jun 28, 2018

polygamma commented Jun 28, 2018

eli-schwartz commented Jun 28, 2018

polygamma commented Jun 30, 2018

eli-schwartz commented Jul 1, 2018

eli-schwartz commented Jul 1, 2018

polygamma commented Jul 1, 2018

polygamma commented Jun 19, 2018 •

edited

polygamma commented Jun 20, 2018 •

edited