Closed Bug 1314795 Opened 4 years ago Closed 4 years ago

introduce build_date to params that defaults to pushdate if passed else current datetime

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla52

People

(Reporter: jlund, Assigned: jlund)

References

Details

Attachments

(2 files)

.... because hook's don't have a pushdate but do need a consistent date for MOZ_BUILD_DATE and dated upload urls.

will add new param to docs
Assignee: nobody → jlund
a couple of notes here.

we need 4 different dates:

1. pushdate. This should continue being supported. Passed in via 'mach decision' cli. e.g. 1478154611 epoch
2. build_date defaults to pushdate. time.time() if not present, like for a hook decision task
3. long build_date/pushdate for index routes. e.g., %Y.%m.%d.%Y%m%d%H%M%S date based on 1478154611 epoch
4. moz_build_date based on build_date/pushdate. e.g. %Y%m%d%H%M%S date based on 1478154611 epoch

I added moz_build_date as a sort of global param just as a convenience so we don't need to infer it from pushdate in every MOZ_BUILD_DATE env value assignment. I haven't made it so you can define it in decision cli or a parameters.yml file because it should be bound to the param 'build_date'. Hence why moz_build_date is not in taskgraph docs but build_date is.
Comment on attachment 8807036 [details]
Bug 1314795 - introduce build_date to params that defaults to pushdate,

https://reviewboard.mozilla.org/r/90318/#review90112

As always, r- from me means "looks good but not quite ready to land" :)

::: taskcluster/taskgraph/decision.py:133
(Diff revision 1)
> +    parameters['moz_build_date'] = time.strftime("%Y%m%d%H%M%S",
> +                                                 time.gmtime(parameters['build_date']))

I think this is fine, but I don't understand the logic behind not putting it in `parameters.rst`.  A parameter is a parameter...

::: taskcluster/taskgraph/parameters.py:25
(Diff revision 1)
> +def set_defaults(parameters):
> +    parameters = copy.deepcopy(parameters)
> +    if not parameters.get('build_date'):
> +        # use the pushdate as build_date if given, else use current time
> +        parameters['build_date'] = parameters['pushdate'] or int(time.time())
> +    # moz_build_date is the build identifier based on build_date
> +    parameters['moz_build_date'] = time.strftime("%Y%m%d%H%M%S",
> +                                                 time.gmtime(parameters['build_date']))
> +    return parameters

I guess this is here so that users hacking with old `parameters.yml` files won't get errors?  I'm not sure that's benefit enough to justify duplicating this code.

What would you think of just verifying that each of a list of required parameters are present, rather than applying defaults?  If one is missing, recommend downloading a new parameters.yml.  Then we can just keep that list in sync with the docs.

(I have crazy ideas about writing tests to verify things in the docs match the code...)
Attachment #8807036 - Flags: review?(dustin) → review-
Comment on attachment 8807036 [details]
Bug 1314795 - introduce build_date to params that defaults to pushdate,

https://reviewboard.mozilla.org/r/90318/#review90112

thanks for the review! I made some comments in line. I'll add moz_build_date to the rst now and wait to hear back on your reply to my set_defaults justification.

> I think this is fine, but I don't understand the logic behind not putting it in `parameters.rst`.  A parameter is a parameter...

I mentioned here why it is not in the rst file: https://bugzilla.mozilla.org/show_bug.cgi?id=1314795#c2

Happy to add it in, I was torn myself on this, but I suppose I felt that moz_build_date should be somewhat hidden and be bound to build_date. I'll add it in though as it's not hidden, it's accessible via `config.paramenters` :)

> I guess this is here so that users hacking with old `parameters.yml` files won't get errors?  I'm not sure that's benefit enough to justify duplicating this code.
> 
> What would you think of just verifying that each of a list of required parameters are present, rather than applying defaults?  If one is missing, recommend downloading a new parameters.yml.  Then we can just keep that list in sync with the docs.
> 
> (I have crazy ideas about writing tests to verify things in the docs match the code...)

this was not to prevent errors but to implement the 3 things mentioned here:

<dustin> how about this:
14:38:53 new parameter, build_date
14:39:21 defaulting to --pushdate when that is supplied, else to int(time.time()) when --nightly, else 0
14:39:47 in fact, let's put it in the right format so we're not replicating that string everywhere

so, granted we don't check for nightly and thus don't default to 0 as I thought using current time as a default everywhere was sane, we 1) create a build_date param 2) try to use pushdate as its value if present 3) creates a moz_build_date that is pre-formated for MOZ_BUILD_DATE

the reason we need both build_date and moz_build_date was commented here: https://bugzilla.mozilla.org/show_bug.cgi?id=1314795#c2
(In reply to Jordan Lund (:jlund) from comment #4)
> Happy to add it in, I was torn myself on this, but I suppose I felt that
> moz_build_date should be somewhat hidden and be bound to build_date. I'll
> add it in though as it's not hidden, it's accessible via
> `config.paramenters` :)

That's basically my thinking.  The docs can mention that it should represent the same time.

> this was not to prevent errors but to implement the 3 things mentioned here:
> 
> <dustin> how about this:
> 14:38:53 new parameter, build_date
> 14:39:21 defaulting to --pushdate when that is supplied, else to
> int(time.time()) when --nightly, else 0
> 14:39:47 in fact, let's put it in the right format so we're not replicating
> that string everywhere
> 
> so, granted we don't check for nightly and thus don't default to 0 as I
> thought using current time as a default everywhere was sane, we 1) create a
> build_date param 2) try to use pushdate as its value if present 3) creates a
> moz_build_date that is pre-formated for MOZ_BUILD_DATE
> 
> the reason we need both build_date and moz_build_date was commented here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1314795#c2

The parameter-loading code (`parameters.py`) isn't used in the decision task (for nightly or on-push).  You've implemented the logic described in the irc conversation in `decision.py`.
Comment on attachment 8807036 [details]
Bug 1314795 - introduce build_date to params that defaults to pushdate,

https://reviewboard.mozilla.org/r/90318/#review90112

> I mentioned here why it is not in the rst file: https://bugzilla.mozilla.org/show_bug.cgi?id=1314795#c2
> 
> Happy to add it in, I was torn myself on this, but I suppose I felt that moz_build_date should be somewhat hidden and be bound to build_date. I'll add it in though as it's not hidden, it's accessible via `config.paramenters` :)

fixed here: https://reviewboard.mozilla.org/r/90318/diff/1-2/
Blocks: 1305096
(In reply to Dustin J. Mitchell [:dustin] from comment #5)
> (In reply to Jordan Lund (:jlund) from comment #4)

> The parameter-loading code (`parameters.py`) isn't used in the decision task
> (for nightly or on-push).  You've implemented the logic described in the irc
> conversation in `decision.py`.

commenting here as I think this reply came outside mozreview.

I've obviously misunderstood. I thought it better to keep the other taskgraph subcommands in sync with the decision subcommand logic. e.g. if pushdate is supplied, use that for build_date. That way devs aren't confused when they read parameters.rst, try to remove build_date from parameters.yml and then subsequent commands, e.g. `taskgraph optimize`, fails as build_date isn't created for them.
Sorry, I'm trying to kick that reply-on-bug habit.

The idea with parameters is that parameters.yml represents a "frozen" image of the inputs to the task-graph generation process.  So decision.py can handle all of the defaults, dervied values, command-line options, etc., and once it's done it writes out a parameters.yml file containing the (now-frozen) parameters it will use to generate the task-graph.  All of the other commands take that file as input.

So, a parameters.yml file that lacks a field -- for example the `pushdate` parameter -- is invalid.  We can decide how to deal with such an invalid file in a way that will minimize frustration for devs.  In general, I'd like devs to download a parameters.yml from a real run and maybe tweak it a little, rather than writing one from scratch, so I'd lean toward bombing out on invalid files with clear suggestions in the error message.

To get back to your patch, I think you can just drop the parameters.py hunk and we'll both be happy?
Comment on attachment 8807036 [details]
Bug 1314795 - introduce build_date to params that defaults to pushdate,

https://reviewboard.mozilla.org/r/90318/#review90112

> this was not to prevent errors but to implement the 3 things mentioned here:
> 
> <dustin> how about this:
> 14:38:53 new parameter, build_date
> 14:39:21 defaulting to --pushdate when that is supplied, else to int(time.time()) when --nightly, else 0
> 14:39:47 in fact, let's put it in the right format so we're not replicating that string everywhere
> 
> so, granted we don't check for nightly and thus don't default to 0 as I thought using current time as a default everywhere was sane, we 1) create a build_date param 2) try to use pushdate as its value if present 3) creates a moz_build_date that is pre-formated for MOZ_BUILD_DATE
> 
> the reason we need both build_date and moz_build_date was commented here: https://bugzilla.mozilla.org/show_bug.cgi?id=1314795#c2

as per bug, reverting changes made to parameters.py: https://reviewboard.mozilla.org/r/90318/diff/2-3/
once https://reviewboard.mozilla.org/r/90318/ lands on central and merges into date, we will need this patch to fix beetmoverscript.
Attachment #8807265 - Flags: review?(kmoir)
Attachment #8807265 - Flags: review?(kmoir) → review+
Comment on attachment 8807036 [details]
Bug 1314795 - introduce build_date to params that defaults to pushdate,

https://reviewboard.mozilla.org/r/90318/#review90232
Attachment #8807036 - Flags: review?(dustin) → review+
https://hg.mozilla.org/projects/date/rev/ffac04c9cf78d4f019e9d0545adca498c33edd4a
Bug 1314795 - introduce build_date to params that defaults to pushdate, r=dustin

https://hg.mozilla.org/projects/date/rev/8916569c32fa65368ee3cbc8e97b49e2c1d81d31
Bug 1314795 - use build_date for beetmoverscript's payload upload_date, CLOSED TREE r=kmoir
Pushed by jlund@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36b1cce63e74
introduce build_date to params that defaults to pushdate, r=dustin
I really need to run these tests before pushing.. Maybe I should add a local hg hook that does that. :)

sorry for the added work Wes.
Flags: needinfo?(jlund)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d9c26e68aa9c4cd880872969affc2121aea3a57
Bug 1314795 - introduce build_date to params that defaults to pushdate, r=dustin
(In reply to Jordan Lund (:jlund) from comment #18)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 7d9c26e68aa9c4cd880872969affc2121aea3a57
> Bug 1314795 - introduce build_date to params that defaults to pushdate,
> r=dustin

mozreview patch + http://people.mozilla.org/~jlund/build_date_lint_fix.patch

checked via: ./mach lint taskcluster/taskgraph
https://hg.mozilla.org/mozilla-central/rev/7d9c26e68aa9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.