ckeditor / ckbuilder

The development repository of CKBuilder, a command line builder for CKEditor 4.
Other
19 stars 11 forks source link

Respect SOURCE_DATE_EPOCH spec #11

Open onlyjob opened 8 years ago

onlyjob commented 8 years ago

ckbuilder puts a buildtime timestamp both in comments and in other part of generated js. This patch make it observe SOURCE_DATE_EPOCH, so to allow for reproducible builds See https://reproducible-builds.org/specs/source-date-epoch/

Bug-Debian: https://bugs.debian.org/819726

fredck commented 8 years ago

One of the purposes of the timestamp is helping on cache invalidation. In other words, browsers will usually take new sources form the server for every new build. This is intentional.

But using the proposed strategy, wouldn't it mean that the timestamp will always be the same, when building from the same environment? For example, two builds with totally different configurations will have the same timestamp if built on the same machine at the same time. Am I right?

If that is right, one of the possibilities we have is providing a command line parameter on the builder so one could pass the desired timestamp. The other option, much more complex so probably unfeasible, would be dropping the timestamp thing altogether and instead creating a hash based on the sources included in the build.

onlyjob commented 8 years ago

The idea it to be able to produce binary identical builds by setting SOURCE_DATE_EPOCH. You can see that default behaviour is not changed unless SOURCE_DATE_EPOCH is set.

When we re-build the same version of package we want to produce identical binaries. New revision of package is built using new(er) time stamp so cache will be properly invalidated on package update. This change helps to produce identical builds when necessary. I hope it make sense.

fredck commented 8 years ago

Ok... I've totally misunderstood the spec... it makes total sense. Thanks for the clarification.

onlyjob commented 8 years ago

No worries. :)

onlyjob commented 8 years ago

I'm slightly disappointed that this improvement is still not merged (so it could not be incorporated into latest release 2.3.1). Is there any doubts or concerns regarding this change?

mlewand commented 8 years ago

@onlyjob Sorry for a long wait, we were quite busy and not able to pay enough attention here. The idea looks fine, I'll just ask you to:

Once that's done we should be fine with accepting your PR, cheers!

onlyjob commented 8 years ago

Unfortunately I have no time or skills to implement tests. Please feel free to take over this pull request and craft it to your preferences. Also it is worth noticing that I am merely forwarding patch that was originally contributed to Debian...

mlewand commented 8 years ago

Hi, I'm sorry, since you're not the author of the code we can't accept it from you in this case. Could you reach the original author of the contribution? Or do you know what license is used for code shared in a referenced place?

boyska commented 8 years ago

I am the author of the code! I am quite flexible about the licensing, so just tell me what you prefer, or whatever I can do to solve the licensing issue.

onlyjob commented 8 years ago

@mlewand, FYI Debian project typically uses the same license as upstream for most patches (unless otherwise is explicitly stated). This contribution is already license-clean.

mlewand commented 8 years ago

I am the author of the code! I am quite flexible about the licensing, so just tell me what you prefer, or whatever I can do to solve the licensing issue.

Since we have original author here, I'd simply ask you @boyska to create a new PR with your code and this will make the situation clear for us. Could you deliver it? :)

onlyjob commented 8 years ago

So much dance around trivial pull request... :( What's wrong with you people?

mlewand commented 8 years ago

@onlyjob we just want to have it nice and clean, there's not much of effort to create a pull request.

onlyjob commented 8 years ago

I've rebased my commit. As far as I'm concerned it is ready to be merged...