Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8238650: Allow to override buildDate with SOURCE_DATE_EPOCH #99

Closed
wants to merge 1 commit into from

Conversation

@bmwiedemann
Copy link

@bmwiedemann bmwiedemann commented Jan 29, 2020

Allow to override buildDate with SOURCE_DATE_EPOCH
in order to make builds reproducible.
See https://reproducible-builds.org/ for why this is good
and https://reproducible-builds.org/specs/source-date-epoch/
for the definition of this variable.

This PR was done while working on reproducible builds for openSUSE.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8238650: Allow to override buildDate with SOURCE_DATE_EPOCH

Download

$ git fetch https://git.openjdk.java.net/jfx pull/99/head:pull/99
$ git checkout pull/99

@bridgekeeper bridgekeeper bot added the oca label Jan 29, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 29, 2020

Hi bmwiedemann, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user bmwiedemann" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 29, 2020

In addition to a signed OCA, we will need a bug filed to track this. Please read the CONTRIBUTING guidelines.

@bmwiedemann
Copy link
Author

@bmwiedemann bmwiedemann commented Jan 29, 2020

/signed

I filed one bug and got review ID : 9063488

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 29, 2020

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 29, 2020

I filed one bug and got review ID : 9063488

OK.

@bmwiedemann
Copy link
Author

@bmwiedemann bmwiedemann commented Feb 7, 2020

So the contributor agreement is done. How can I find the bug?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 7, 2020

The OCA needs to be validated and recorded. As for the bug, it has been transferred to the JDK project as JDK-8238650.

in order to make builds reproducible.
See https://reproducible-builds.org/ for why this is good
and https://reproducible-builds.org/specs/source-date-epoch/
for the definition of this variable.

Signed-off-by: Bernhard M. Wiedemann <bwiedemann@suse.de>
@bmwiedemann bmwiedemann changed the title Allow to override buildDate with SOURCE_DATE_EPOCH 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH Feb 7, 2020
@openjdk openjdk bot added the rfr label Feb 7, 2020
@bmwiedemann
Copy link
Author

@bmwiedemann bmwiedemann commented Feb 7, 2020

The OCA needs to be validated and recorded.

https://www.oracle.com/technetwork/community/oca-486395.html#w now has
Bernhard Wiedemann - OpenJDK - bmwiedemann

As for the bug, it has been transferred to the JDK project as JDK-8238650.

Thanks, somehow I had not got an email notification for that.
Also worth noting that this PR only fixes one of the sources of non-determinism in openjfx. Do I have to open a separate bug for each of them?

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 7, 2020

Webrevs

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 7, 2020

Do I have to open a separate bug for each of them?

Every fix (meaning each pull request) needs a unique bug ID.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 7, 2020

/reviewers 2

@kevinrushforth kevinrushforth self-requested a review Feb 7, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Feb 7, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 7, 2020

As an optional override, I am OK with the concept of having a way for the build to be reproducible.

FWIW, I have scripts that will unpack the modular jar files and diff each class as well as doing the same for a src.zip, and it's pretty easy to tell if only VersionInfo (which is the class that records the time stamps) has changed.

I note that in practice, this is useful for a certain class of builds (e.g., CI or nightly test builds), but each released build is necessarily going to be different because you want a unique time stamp and build number associated with it.

I will review this (probably some time next week) and would like @johanvos to review as well.

@bmwiedemann
Copy link
Author

@bmwiedemann bmwiedemann commented Feb 8, 2020

FWIW, I have scripts that will unpack the modular jar files and diff each class

I agree that such specialized diff tools have some value, yet, there are also some limitations and downsides to them. E.g. you cannot simply tell another party what the expected sha256sum of a build result is.

https://www.suse.com/c/?p=42014 also has a section on problems with "the use of specialized comparison tools like [openSUSE's] ‘build-compare‘ "

I probably should write an FAQ entry about that topic...

each released build is necessarily going to be different because you want a unique time stamp and build number associated with it.

For release builds, it is important that other people can take the released sources and reproduce the same original binaries with the same release number (and ideally same timestamps) to easily verify that the build was clean (not corrupted by bad CPUs/RAM/HDDs or someone messing with the build machine).
I heard, some people even use that to save network bandwidth: add a small patch locally+remotely, build it locally, tell the world the new build hash, but have others upload their binaries with the right hash.

@bmwiedemann
Copy link
Author

@bmwiedemann bmwiedemann commented Feb 25, 2020

Hi, did you find time to review this?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 25, 2020

No, I'm pretty backed up on reviews. It's on my queue, though.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Finally getting to this (sorry for the delay). This seems OK to me, as it is not an intrusive change. I do not think that actually setting this property for production builds is a good idea, so I recommend against that (ultimately that will be up to Gluon or whoever produces builds).

I'll test it once you change it from an env variable to a gradle property.

@@ -539,7 +539,7 @@ if (jfxReleasePatchVersion == "0") {
defineProperty("RELEASE_VERSION", relVer)
defineProperty("RELEASE_VERSION_PADDED", "${jfxReleaseMajorVersion}.${jfxReleaseMinorVersion}.${jfxReleaseSecurityVersion}.${jfxReleasePatchVersion}")

def buildDate = new java.util.Date()
def buildDate = System.getenv("SOURCE_DATE_EPOCH") == null ? new java.util.Date() : new java.util.Date(1000 * Long.parseLong(System.getenv("SOURCE_DATE_EPOCH")))
Copy link
Member

@kevinrushforth kevinrushforth Feb 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be defined as a gradle property using defineProperty like this:

defineProperty("SOURCE_DATE_EPOCH", "")

You can then test for SOURCE_DATE_EPOCH == ""

You would pass it into the build via gradle -PSOURCE_DATE_EPOCH=...

I also recommend wrapping the line since it's pretty long.

While you are at it, can you add BUILD_TIMESTAMP to the list of properties that are logged? Look for log.quiet.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 14, 2020

@bmwiedemann are you planning to pursue this PR?

@bmwiedemann
Copy link
Author

@bmwiedemann bmwiedemann commented Apr 15, 2020

Long term, yes.

I am currently low on time and my Java-foo is not that great, so I was hoping that someone would pick it up.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented May 8, 2020

I note that the JDK build recently implemented this feature in JDK 15 with JDK-8244592.

@kevinrushforth kevinrushforth marked this pull request as draft Aug 26, 2020
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Aug 26, 2020

Converted to a Draft PR, so that it will not show up as rfr. Go ahead and resubmit it when you are ready to proceed.

@openjdk openjdk bot removed the rfr label Aug 26, 2020
@jgneff
Copy link
Member

@jgneff jgneff commented Mar 8, 2021

I am currently low on time and my Java-foo is not that great, so I was hoping that someone would pick it up.

@bmwiedemann Thank you for getting this important change started. If you can finish this pull request, I'll follow up with some additional changes to support reproducible builds. Otherwise, if you're still short on time, I can pick up where you left off. So far I managed to create reproducible Java artifacts — all but the native shared libraries (and the javafx.graphics.jmod file that includes them).

@bmwiedemann
Copy link
Author

@bmwiedemann bmwiedemann commented Mar 9, 2021

@jgneff feel free to pick it up.

When linking shared libraries, ensure that .o files are listed in deterministic order - i.e. sort(glob(*.c)) and check if individual .c or .o files vary between builds.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 17, 2021

@bmwiedemann Unknown command signed - for a list of valid commands use /help.

@jgneff
Copy link
Member

@jgneff jgneff commented Mar 26, 2021

When linking shared libraries, ensure that .o files are listed in deterministic order - i.e. sort(glob(*.c)) and check if individual .c or .o files vary between builds.

Wow, that tip was priceless! Thank you, Bernhard. It took forever to figure out where to add it, but simply adding .sort() to the list of object files for the linker did the trick:

index 8f1e04a833..9181ccc7fb 100644
--- a/buildSrc/src/main/groovy/com/sun/javafx/gradle/LinkTask.groovy
+++ b/buildSrc/src/main/groovy/com/sun/javafx/gradle/LinkTask.groovy
@@ -49,7 +49,7 @@ class LinkTask extends DefaultTask {
                 args("$lib");
             }
             // Exclude parfait files (.bc)
-            args(objectDir.listFiles().findAll{ !it.getAbsolutePath().endsWith(".bc") });
+            args(objectDir.listFiles().sort().findAll{ !it.getAbsolutePath().endsWith(".bc") });
             if (project.IS_WINDOWS) {
                 args("/out:$lib");
             } else {

We have reproducible builds! (Well, we have them on Linux, at least, and only after running strip-nondeterminism on the JMOD files, but that's still major progress.) I'll follow up with a pull request after checking whether I can get reproducible builds on macOS and Windows, too.

@bmwiedemann
Copy link
Author

@bmwiedemann bmwiedemann commented Mar 27, 2021

objectDir.listFiles().sort()

Now the interesting question is, if it is possible to change listFiles to always return deterministic output or if we have to patch an infinite number of callers instead. For those cases where people do not care about order, one could add listUnsortedFiles with the old behaviour. There is a small performance impact, but usually that is rather small (below 4%) compared to interacting with the filesystem and processing the files returned.

@jgneff
Copy link
Member

@jgneff jgneff commented Mar 27, 2021

Now the interesting question is, if it is possible to change listFiles to always return deterministic output ...

Yes, that would be interesting, if listFiles() were defined by the Gradle build tool. I think it originates, though, in the Java java.io.File.listFiles API and is made available to Gradle through the Groovy programming language. The API states, "There is no guarantee that the name strings in the resulting array will appear in any specific order; they are not, in particular, guaranteed to appear in alphabetical order." That API was defined in December 1998 with Java version 1.2, so there's no changing it now.

@jgneff
Copy link
Member

@jgneff jgneff commented Mar 27, 2021

By the way, we have reproducible builds on Linux, macOS, and Windows now! (Well, still using strip-nondeterminism on the JMOD files, and as long as you build from the same file system path, thanks to com.sun.javafx.css.parser.Css2Bin. Sigh. This rabbit hole is deep.)

@bmwiedemann
Copy link
Author

@bmwiedemann bmwiedemann commented Mar 27, 2021

The API states, "There is no guarantee that the name strings in the resulting array will appear in any specific order; they are not, in particular, guaranteed to appear in alphabetical order." That API was defined in December 1998 with Java version 1.2, so there's no changing it now.

Sorted output still complies with that API, though.
If changing it at the Java level is too impactful, there could still be a sort-wrapper in Gradle or some other layer.

@jgneff
Copy link
Member

@jgneff jgneff commented Mar 28, 2021

Sorted output still complies with that API, though.

Well, that's true. I hadn't thought of it that way.

I have tried twice and failed to add you as a contributor to my follow-up pull request. The only option left is the syntax "contributor add J. Duke duke@openjdk.org", but I'm not sure whether you want the e-mail address in your GitHub profile made public. Let me know whether I should pursue the problem.

If you're added, it puts your name and address in the commit log with Co-authored-by like this example:

commit 8adbc673d095607e8a6109fbb951fa17b9d6caad (tag: 17+3)
Author: Alexander Matveev <almatvee@openjdk.org>
Date:   Mon Mar 15 20:57:19 2021 +0000

    8257895: Allow building of JavaFX media libs for Apple Silicon
    
    Co-authored-by: Johan Vos <jvos@openjdk.org>
    Reviewed-by: jvos, kcr

It might be useful to have a previous commit if you plan to contribute to the OpenJFX project in the future.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 5, 2021

I think PR can be closed now, in favor of PR #422

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants