Bug #12566
ikiwiki image size specification makes the ISO build unreproducible
100%
Description
In [[!img introduction_to_gnome_and_the_tails_desktop/keyboard.png size="267x" link="no" alt="Menu in the top-right corner of the desktop to switch between different keyboard layouts"]], size="267x" makes ikiwiki generate files called 267x-keyboard.png in an unreproducible manner.
Is this 267x thing correct, or a typo for 267px?
Subtasks
Related issues
|
Related to Tails - |
Resolved | 2017-05-31 |
History
#1 Updated by intrigeri 2017-05-19 09:50:27
- Assignee changed from sajolida to anonym
- Type of work changed from End-user documentation to Code
Actually that’s valid ikiwiki syntax (https://ikiwiki.info/ikiwiki/directive/img/) so it’s not sajolida’s problem. Reassigning to the person committed to do the bulk of the work on the reproducible builds deliverable.
The generated PNG has an embedded timestamp.
#2 Updated by anonym 2017-05-31 14:51:56
Ikiwiki does the resizeing in IkiWiki/Plugin/img.pm, using an interface to ImageMagick. I think Ikiwiki just have to tell PerlMagick to do the equivalent of convert +set date:create +set date:modify -define png:exclude-chunk=time to fix this.
Any way, it’s so easy for us to fix this by simply replacing the current image with version rescaled to 267x252, so I cannot really justify looking deeper into this (if I was more fluent in Perl, so this wouldn’t take a lot of time, I’d consider doing it, though). Bonus: we won’t get an identical (modulo the time stamp) resized PNG generated for each locale we translate for.
#3 Updated by anonym 2017-05-31 15:19:38
anonym wrote:
> I think Ikiwiki just have to tell PerlMagick to do the equivalent of convert +set date:create +set date:modify -define png:exclude-chunk=time to fix this.
FWIW, from my super quick look it seems the two +set ... things can be accomplished with:
--- a/IkiWiki/Plugin/img.pm
+++ b/IkiWiki/Plugin/img.pm
@@ -174,6 +174,9 @@ sub preprocess (@) {
my $r = $im->Read("$format:$srcfile\[$pagenumber]");
error sprintf(gettext("failed to read %s: %s"), $file, $r) if $r;
+ $im->Set('date:create' => NULL);
+ $im->Set('date:modify' => NULL);
+
if (! defined $im->Get("width") || ! defined $im->Get("height")) {
error sprintf(gettext("failed to get dimensions of %s"), $file);
}
but perhaps we only want to do this if SOURCE_DATE_EPOCH is set (and then set it to that value instead of NULL?). And I couldn’t find how to do -define png:exclude-chunk=time.
#4 Updated by intrigeri 2017-05-31 15:34:58
> Ikiwiki does the resizeing in IkiWiki/Plugin/img.pm, using an interface to ImageMagick. I think Ikiwiki just have to tell PerlMagick to do the equivalent of convert +set date:create +set date:modify -define png:exclude-chunk=time to fix this.
Thanks for diving into the (Perl) code!
> Any way, it’s so easy for us to fix this by simply replacing the current image with version rescaled to 267x252, so I cannot really justify looking deeper into this (if I was more fluent in Perl, so this wouldn’t take a lot of time, I’d consider doing it, though).
Regarding the short-term: I agree in principle that this would be a perfectly acceptable short-term fix. But this image is used in two different places on the testing branch: in one case it’s downscaled, in the other case it’s displayed full size (just reading the source, I didn’t check the built result). So “simply replacing the current image with version rescaled to 267x252” may not be OK. As far as 3.0 is concerned, we might have to duplicate this image (ugly, sad, but well, if we have a better long-term plan it’s fine with me).
Regarding the long-term: in practice this means something like “when a tech writer introduces such a reproducibility issue, Foundation Team or RM will notice it and resize images”. I expect it’ll happen very rarely and won’t be a big burden in general. But it might also happen at the worst possible time, e.g. when merging release notes with new screenshots into the stable branch, at the last minute before building the final ISO… and then it can be stressful for the RM to spend time on this and make the critical path to the release longer. I’d rather avoid adding such stress possibilities to the release process. I am fluent in Perl, I know the ikiwiki code base, we have time left on this project, and we promised to fix as much stuff as we could upstream: so I could attempt a proper fix, perhaps with a time limit set in advance, e.g. no more than 8 hours of work. What do you think?
> Bonus: we won’t get an identical (modulo the time stamp) resized PNG generated for each locale we translate for.
I’m not sure I get this point. In theory, translators should add a localized picture every time the screenshot is different in the language they’re translating to, so ideally we want a different picture per language.
#5 Updated by anonym 2017-05-31 15:50:32
- Assignee changed from anonym to intrigeri
- QA Check set to Info Needed
intrigeri wrote:
> > Any way, it’s so easy for us to fix this by simply replacing the current image with version rescaled to 267x252, so I cannot really justify looking deeper into this (if I was more fluent in Perl, so this wouldn’t take a lot of time, I’d consider doing it, though).
>
> Regarding the short-term: I agree in principle that this would be a perfectly acceptable short-term fix. But this image is used in two different places on the testing branch: in one case it’s downscaled, in the other case it’s displayed full size (just reading the source, I didn’t check the built result).
Indeed.
> So “simply replacing the current image with version rescaled to 267x252” may not be OK. As far as 3.0 is concerned, we might have to duplicate this image (ugly, sad, but well, if we have a better long-term plan it’s fine with me).
I propose this short term fix: let’s drop the resizing and show the image in full size in both instances. I had a look, and showing the image in full size on the startup options page doesn’t look weird to me at least.
Agreed?
> Regarding the long-term: in practice this means something like “when a tech writer introduces such a reproducibility issue, Foundation Team or RM will notice it and resize images”. I expect it’ll happen very rarely and won’t be a big burden in general. But it might also happen at the worst possible time, e.g. when merging release notes with new screenshots into the stable branch, at the last minute before building the final ISO… and then it can be stressful for the RM to spend time on this and make the critical path to the release longer. I’d rather avoid adding such stress possibilities to the release process.
Very good point.
> I am fluent in Perl, I know the ikiwiki code base, we have time left on this project, and we promised to fix as much stuff as we could upstream: so I could attempt a proper fix, perhaps with a time limit set in advance, e.g. no more than 8 hours of work. What do you think?
Sounds good to me! Let me know if you want me to create this ticket for you!
> > Bonus: we won’t get an identical (modulo the time stamp) resized PNG generated for each locale we translate for.
>
> I’m not sure I get this point. In theory, translators should add a localized picture every time the screenshot is different in the language they’re translating to, so ideally we want a different picture per language.
Got it, I wasn’t aware of this.
#6 Updated by intrigeri 2017-05-31 15:53:10
> but perhaps we only want to do this if SOURCE_DATE_EPOCH is set (and then set it to that value instead of NULL?). And I couldn’t find how to do -define png:exclude-chunk=time.
ikiwiki doesn’t do SOURCE_DATE_EPOCH at all (yet), but:
- look for
if ($config{deterministic}) - perhaps using Unix epoch is good enough
- in some areas (when it’s not particularly expensive) the output was made deterministic by default
#7 Updated by intrigeri 2017-05-31 15:56:01
- Assignee changed from intrigeri to anonym
- QA Check changed from Info Needed to Dev Needed
> I propose this short term fix: let’s drop the resizing and show the image in full size in both instances.
ACK
>> I am fluent in Perl, I know the ikiwiki code base, we have time left on this project, and we promised to fix as much stuff as we could upstream: so I could attempt a proper fix, perhaps with a time limit set in advance, e.g. no more than 8 hours of work. What do you think?
> Sounds good to me! Let me know if you want me to create this ticket for you!
Yes, please.
#8 Updated by intrigeri 2017-05-31 16:17:57
- related to
Feature #12625: Make Ikiwiki resize images deterministically added
#9 Updated by intrigeri 2017-05-31 16:18:18
- Subject changed from Weird ikiwiki image size specification makes the ISO build unreproducible to ikiwiki image size specification makes the ISO build unreproducible
#10 Updated by anonym 2017-05-31 16:27:47
- Subject changed from ikiwiki image size specification makes the ISO build unreproducible to Weird ikiwiki image size specification makes the ISO build unreproducible
- Status changed from Confirmed to In Progress
- % Done changed from 0 to 30
- QA Check changed from Dev Needed to Ready for QA
- Feature Branch set to bugfix/12566-dont-resize-image
intrigeri wrote:
> > I propose this short term fix: let’s drop the resizing and show the image in full size in both instances.
>
> ACK
Implemented in the feature branch. I’ll just make sure this actually works before asking for it to be merged.
> >> I am fluent in Perl, I know the ikiwiki code base, we have time left on this project, and we promised to fix as much stuff as we could upstream: so I could attempt a proper fix, perhaps with a time limit set in advance, e.g. no more than 8 hours of work. What do you think?
>
> > Sounds good to me! Let me know if you want me to create this ticket for you!
>
> Yes, please.
#11 Updated by anonym 2017-05-31 16:28:32
- Subject changed from Weird ikiwiki image size specification makes the ISO build unreproducible to ikiwiki image size specification makes the ISO build unreproducible
#12 Updated by anonym 2017-05-31 18:14:11
- Assignee changed from anonym to intrigeri
- % Done changed from 30 to 50
anonym wrote:
> intrigeri wrote:
> > > I propose this short term fix: let’s drop the resizing and show the image in full size in both instances.
> >
> > ACK
>
> Implemented in the feature branch. I’ll just make sure this actually works before asking for it to be merged.
Works for me locally! Please review’n’merge into testing!
#13 Updated by intrigeri 2017-06-01 07:07:32
- % Done changed from 50 to 60
Code review passes, built the website locally and indeed it looks OK without resizing. I’ve set up https://jenkins.tails.boum.org/job/reproducibly_build_Tails_ISO_bugfix-12566-dont-resize-image/ to check reproducibility.
#14 Updated by intrigeri 2017-06-01 11:28:24
- Status changed from In Progress to Fix committed
- % Done changed from 60 to 100
Applied in changeset commit:49dd3889caa1664c7511eeaf250852168a898b56.
#15 Updated by intrigeri 2017-06-01 11:29:15
- Assignee deleted (
intrigeri) - QA Check changed from Ready for QA to Pass
#16 Updated by intrigeri 2017-06-12 16:07:11
- Status changed from Fix committed to Resolved