Bug 697484 - Make build of ghostscript deterministic
Summary: Make build of ghostscript deterministic
Status: RESOLVED FIXED
Alias: None
Product: Ghostscript
Classification: Unclassified
Component: Build Process (show other bugs)
Version: unspecified
Hardware: PC Linux
: P4 normal
Assignee: Chris Liddell (chrisl)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-16 14:54 UTC by Stefan Brüns
Modified: 2017-02-10 02:17 UTC (History)
2 users (show)

See Also:
Customer:
Word Size: ---


Attachments
Use fixed time in timestamp (1.68 KB, patch)
2017-01-16 14:54 UTC, Stefan Brüns
Details | Diff
Sort resources by name (2.57 KB, patch)
2017-01-16 14:56 UTC, Stefan Brüns
Details | Diff
Sort resources by name (2.57 KB, patch)
2017-01-31 11:38 UTC, Stefan Brüns
Details | Diff
Patch v2 for real (2.89 KB, patch)
2017-02-09 12:36 UTC, Stefan Brüns
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Brüns 2017-01-16 14:54:32 UTC
Created attachment 13299 [details]
Use fixed time in timestamp

Currently each build of ghostscript produces a slightly different gs binary/ghostscript library, even if all tools/sources are unchanged.

This is somewhat related to https://bugs.ghostscript.com/show_bug.cgi?id=696765, although it is different as it is not about GS output, but the GS build itself.

GS build is deterministic save the generation of the %ROM resource, which is created during the build process. There are two aspects which are not deterministic:

1. mkromfs embeds the current time whenever it is executed
2. mkromfs adds the individual resources, e.g. Fonts/*, in the order as returned by the readdir() library call.
Comment 1 Stefan Brüns 2017-01-16 14:56:10 UTC
Created attachment 13300 [details]
Sort resources by name
Comment 2 Chris Liddell (chrisl) 2017-01-19 02:58:17 UTC
The sorting resources patch is fine by me, so I'll put that in soon.

I know it's considered "standard" now, but I have to say that I am *fundamentally* opposed to the SOURCE_DATE_EPOCH thing (not just for us, but in general). If a build process is specifically designed to include the current date, it should include the current date. A "solution" that allows the real time of the build to replaced with any random date is potentially just misleading.

FWIW, I'm not at all sure *why* we embed the date in the romfs. My first step is going to be to establish why. If there's no good reason, then we'll just remove the date stamp. If there *is* a good reason then, I'd be tempted to check for the existence of SOURCE_DATE_EPOCH and if it is set, then set the date to all zeros - that way, there can be *no* confusion that the date is "real".
Comment 3 Stefan Brüns 2017-01-19 07:30:41 UTC
(In reply to Chris Liddell (chrisl) from comment #2)
> The sorting resources patch is fine by me, so I'll put that in soon.
 
Just a small Heads Up, the patch omits freeing the string array. For mkromfs this is likely not a big issue, but Valgrind or some static code checkers may be unhappy about that.

> I know it's considered "standard" now, but I have to say that I am
> *fundamentally* opposed to the SOURCE_DATE_EPOCH thing (not just for us, but
> in general). If a build process is specifically designed to include the
> current date, it should include the current date. A "solution" that allows
> the real time of the build to replaced with any random date is potentially
> just misleading.

Unfortunately most build processes are not aware they are embedding a semi-random timestamp (depending on existence of some sort of real-time clock, speed of harddisk, ...).

> FWIW, I'm not at all sure *why* we embed the date in the romfs. My first
> step is going to be to establish why. If there's no good reason, then we'll
> just remove the date stamp. If there *is* a good reason then, I'd be tempted
> to check for the existence of SOURCE_DATE_EPOCH and if it is set, then set
> the date to all zeros - that way, there can be *no* confusion that the date
> is "real".

*If* there currently is a reason for the timestamp beyond being pure informational, this dependency should be removed, as it is prone to break anyway, in misterious ways.
Comment 4 Stefan Brüns 2017-01-31 11:38:17 UTC
Created attachment 13336 [details]
Sort resources by name

v2: free strduped string, file list
Comment 5 Stefan Brüns 2017-01-31 13:01:08 UTC
Regarding the timestamp in the romfs:
1. It is used in base/gsiorom.c to determine if the romfs is a dummy. This could be changed to "if (!gs_romfs[0]) { ...", as the first entry in the dummy rom is initialized to zero.

2. It is passed on in the st_mtime/st_ctime fields, and later used by zstatus in psi/zfile.c. zstatus itself does not interpret the fields, but puts the the values on the interpreter stack.

As the interpretation of the mtime/ctime is not really specified (according to PSRM), I would say filling in 0/0 would be ok.
Comment 6 Chris Liddell (chrisl) 2017-02-07 08:25:43 UTC
OKay, sorry to mess you around on this. The more I've thought about it, the more I'm okay (not happy, exactly, but "okay") with using SOURCE_DATE_EPOCH in this instance. So, I am inclined to accept the patches as they stand.
Comment 8 Stefan Brüns 2017-02-09 11:58:56 UTC
(In reply to Chris Liddell (chrisl) from comment #7)
> Patches applied in:
> 
> http://git.ghostscript.com/?p=ghostpdl.git;a=commitdiff;h=98696f718b
> http://git.ghostscript.com/?p=ghostpdl.git;a=commitdiff;h=aa28186288

The second patch leaks memory, I had sent a second version of the patch which cleans up allocated memory.
Comment 9 Chris Liddell (chrisl) 2017-02-09 12:20:26 UTC
(In reply to Stefan Brüns from comment #8)
> (In reply to Chris Liddell (chrisl) from comment #7)
> > Patches applied in:
> > 
> > http://git.ghostscript.com/?p=ghostpdl.git;a=commitdiff;h=98696f718b
> > http://git.ghostscript.com/?p=ghostpdl.git;a=commitdiff;h=aa28186288
> 
> The second patch leaks memory, I had sent a second version of the patch
> which cleans up allocated memory.

Damn, I did use the second version of the "Sort resources by name" patch - looking at the two, the obsolete patch and the current one are identical.
Comment 10 Chris Liddell (chrisl) 2017-02-09 12:35:51 UTC
This should be sufficient:
http://git.ghostscript.com/?p=user/chrisl/ghostpdl.git;a=commitdiff;h=5b196e5f
Comment 11 Stefan Brüns 2017-02-09 12:36:57 UTC
Created attachment 13366 [details]
Patch v2 for real

Did upload the wrong file, this one should be correct
Comment 12 Chris Liddell (chrisl) 2017-02-10 00:20:39 UTC
In the interest of clarity and maintainability, I prefer having it this way:
https://git.ghostscript.com/?p=user/chrisl/ghostpdl.git;a=commitdiff;h=c7d5567fbc