Use egrep to find repository components in arbitrary positions. #532
Conversation
The existing implementation assumes that the repositories in question are part of the sources.list on a line of their own when they may exist as one component among several on the same line. This trades the fixed string check for regular expression that will find the desired component regardless of its position among multiple components or if it is on a line by itself.
|
The problem with interpreting the previous fixed string as a regex is that the fixed part does contain characters which have special meaning in a regex. Without escaping them the result is not necessarily correct. Therefore I don't think we should use a regex here.
http://build.ros.org/job/Kbin_djv8_dJv8__cob_substitute__debian_jessie_arm64__binary/ |
Such as `non-free`...
That's a fair point, although because I've split the pattern lines into a separate variable we could apply that escaping. I think the assumption that these repositories would always be added on unique lines is an unnecessarily brittle one even if we have the ability to enforce it for our own images at the moment. |
It's unlikely these patterns would appear with anything other than a literal '.' in these files, and in fact there was an existing pattern in a sed command which also used wildcard dots rather than quoted dots but this replaces all wildcard dots in patterns with the expected quoted dot.
The doubled trailing slash appears to've been added in c95cbb3. Since there is no explanation for it in the commit message I assume it was added unintentionally and have removed it. Additionally, a review of the apt line in our current osrf/ubuntu_arm* images where ports.ubuntu.com is likely to appear has no trailing slashes, which means we'd still end up with doubled apt repositories, but perhaps becauase the urls are distinct we don't get warnings for it. To guard against future changes, let's just make the trailing slash optional for all patterns.
|
I've updated this PR to quote literal dots in patterns. I also noticed that a sed substitution was using wildcard dots so I quoted those for good measure. While reading the patterns closely, I noticed that there was a doubled trailing slash on one line, first introduced in c95cbb3 with no explanation. I figured I'd remove it and thought to check the current text of those lines in our live images. shows no trailing slash at all. Rather than try and tightly couple the string literal to the particular upstream image. I've just opted to make trailing slashes optional for all patterns. A survey of upstream ubuntu and debian docker images shows that ubuntu defaults to a trailing slash while debian does not. |
| }@ | ||
| RUN @(' && '.join(commands)) | ||
| # Hit cloudfront mirror because of corrupted packages on fastly mirrors (https://github.com/ros-infrastructure/ros_buildfarm/issues/455) | ||
| # You can remove this line to target the default mirror or replace this to use the mirror of your preference | ||
| RUN sed -i 's/httpredir.debian.org/cloudfront.debian.net/' /etc/apt/sources.list | ||
| RUN sed -i 's/httpredir\.debian\.org/cloudfront.debian.net/' /etc/apt/sources.list |
There was a problem hiding this comment.
Is there a reason to only escape two of the dots?
There was a problem hiding this comment.
Is there a reason to only escape two of the dots?
This is a sed substitution string so only the portion within the first // is treated as a regular expression. The text after the second /, cloudfront.debian.net is the replacement string and is already a literal (if it included back-references like \1 it wouldn't be truly a literal, but the '.' characters would still be literal).
There was a problem hiding this comment.
Oh, I see now. Too much different tools going on 😉
This is a proposed enhancement of #530
The existing implementation assumes that the repositories in question are part of the sources.list on a line of their own when they may exist as one component among several on the same line. This trades the fixed string check for regular expression that will find the desired component regardless of its position among multiple components or if it is on a
line by itself.
@dirk-thomas did you have a Debian test case that you were using for #530? I've only tested this using generate_devel_script.py for indigo fanuc on ubuntu trusty.