Project

General

Profile

Bug #15421

Improve the implementation of the #15400 bugfix

Added by intrigeri over 1 year ago. Updated over 1 year ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
03/16/2018
Due date:
% Done:

100%

Feature Branch:
bug/15400-python-lib-breaks-reproducibility
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

See #15400#note-14 and follow ups.


Related issues

Related to Tails - Bug #15400: Python files installed in /usr/local/lib break reproducibility Resolved 03/13/2018

Associated revisions

Revision 561a7fc8 (diff)
Added by segfault over 1 year ago

Fix 00-install-tailslib in case of multiple matches

Will-fix: #15421

Revision 5ae4c1d6 (diff)
Added by intrigeri over 1 year ago

Simplify implementation (refs: #15421).

Instead of going through lengths trying to determine whether we can do what we
want, just try to do it; and if it fails, well, it should be quite clear why.

Revision 1e8af1e7 (diff)
Added by intrigeri over 1 year ago

Drop comment that's not needed anymore (refs: #15421).

Now that all the surrounding code is gone, it's quite obvious
what we're doing.

Revision 80d36fea (diff)
Added by intrigeri over 1 year ago

Don't hide strip_nondeterminism_wrapper errors (refs: #15421)

If it fails e.g. because the glob does not match anything,
we want to learn about it.

History

#1 Updated by intrigeri over 1 year ago

  • Related to Bug #15400: Python files installed in /usr/local/lib break reproducibility added

#2 Updated by intrigeri over 1 year ago

  • Status changed from Confirmed to In Progress
  • % Done changed from 0 to 10

#3 Updated by intrigeri over 1 year ago

  • Assignee changed from intrigeri to segfault
  • QA Check changed from Ready for QA to Dev Needed

First, this branch was originally based on devel, but the one we merged for 3.6.1 had to be rebased on stable so the topic branch (still based on devel) now has duplicate commits. Please rebase it on top of stable and ensure there's no duplicate commit.

Then, following-up on the beginning of the discussion that happened on #15400:

segfault wrote:

intrigeri wrote:

Thanks!

  • Code review:
    • wrt. a867dfd898275d60c935c739a676e6ddb99e2847 I don't really get it: why not drop the quotes instead? You're not quoting the same variable a few lines above so it has to be safe for unquoted usage anyway.

You are right. I thought the quotes in strip_nondeterminism_wrapper() in the following line were the cause:

strip-nondeterminism "${@}"

But indeed in a quick test (I didn't run the build yet) it seems to work if I just remove the quotes in 00-install-tailslib. I don't understand why, but I'm used to not understanding why bash does weird things.

commit:b3429630495744cb9d88471c6389d340db399df2 LGTM.

  • I'm not super happy with the fact the implementation will break if the glob ever matches more than one file. Something based on ls or find and loops over the matches would be more robust.

It should not break. I already tested this before, but I see that I should have noted it in the commit message: Both realpath and strip-nondeterminism can take multiple filenames, and then do its work on all of them. So if the glob matches multiple files, they will simply all be stripped.

OK, but -f only supports being passed exactly one path so it'll break if the glob ever matches 0 or 2+ files. Try running this with /bin/sh:

package_glob_pattern="a*" 
workdir=$(mktemp -d)
cd "$workdir" 
touch a1 a2
[ ! -f ${package_glob_pattern} ]

… and you'll see a shell runtime syntax error ("unexpected operator").

#4 Updated by segfault over 1 year ago

  • Assignee changed from segfault to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

Please rebase it on top of stable and ensure there's no duplicate commit.

Done.

OK, but -f only supports being passed exactly one path so it'll break if the glob ever matches 0 or 2+ files.

If the glob matches 0 files, it will not expand but remain as a pattern, so the -f test will work. But you're right that it will break in the 2+ files case, I missed that. I pushed a commit which should fix that.

#5 Updated by intrigeri over 1 year ago

  • Assignee changed from intrigeri to segfault
  • % Done changed from 10 to 50
  • QA Check changed from Ready for QA to Info Needed

OK, that's better! But wait, what would be the drawback of the simpler:

    # Strip nondeterministic information from tailslib
    package_glob_pattern="/usr/local/lib/python3.*/dist-packages/Tailslib*.egg" 
    strip_nondeterminism_wrapper --type zip ${package_glob_pattern} 2>/dev/null

?

#6 Updated by segfault over 1 year ago

  • Assignee changed from segfault to intrigeri

intrigeri wrote:

OK, that's better! But wait, what would be the drawback of the simpler:

[...]

?

A less helpful error message in the case that the file is not found. Or do you only mean to replace this line?

strip_nondeterminism_wrapper --type zip "$(realpath ${package_glob_pattern})" 2>/dev/null

I guess that can be simplified, I don't remember why I used realpath there. I'm also getting frustrated that we are still discussing these < 10 lines of shell script. Looking forward to when we ported the shell library to Python, so I can do things like this without making a bunch of errors / unnecessary complex things.

#7 Updated by segfault over 1 year ago

segfault wrote:

I guess that can be simplified, I don't remember why I used realpath there.

I remember now, I tried it with "${package_glob_pattern}" and that didn't work, because the glob pattern doesn't expand if quoted. But your version seems to work.

#8 Updated by intrigeri over 1 year ago

  • Feature Branch changed from segfault:bug/15400-python-lib-breaks-reproducibility to bug/15400-python-lib-breaks-reproducibility

intrigeri wrote:

OK, that's better! But wait, what would be the drawback of the simpler:
[...]
?

A less helpful error message in the case that the file is not found.

I see; that's true but I don't think it's worth the hassle of getting the check right (given the time we've on it, it's now very clear that it's not a trivial task). I've pushed to our CI a few commits (based on your branch) that simplify this script a lot and if it works there I'll reassign to anonym for merging.

I'm also getting frustrated that we are still discussing these < 10 lines of shell script. Looking forward to when we ported the shell library to Python, so I can do things like this without making a bunch of errors / unnecessary complex things.

Fully agreed. As said elsewhere, I think it's a waste of both your time and mine for me to mentor you through shell scripting in 2018. Now, of course the very script that installs our Python library cannot use it itself (and we would need strip_nondeterminism_wrapper in there to rewrite this script in Python). So you can now forget about this ticket!

#9 Updated by intrigeri over 1 year ago

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Info Needed to Ready for QA

#10 Updated by bertagaz over 1 year ago

  • Assignee changed from anonym to bertagaz

Taking this over, for it to get in 3.7 in time.

#11 Updated by bertagaz over 1 year ago

  • Status changed from In Progress to Fix committed
  • Assignee deleted (bertagaz)
  • % Done changed from 50 to 100
  • QA Check changed from Ready for QA to Pass

bertagaz wrote:

Taking this over, for it to get in 3.7 in time.

And it looks good and passes the test suite so it's merged.

#12 Updated by bertagaz over 1 year ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF