Fwd: wip/new-model review

["Jasper St. Pierre" <jstpierre mecheye net>]

(resending to the list because moderation failed. Colin, some extra notes are added)

On Sun, Jul 7, 2013 at 11:26 AM, Colin Walters <walters verbum org> wrote:

Hello ostree-list,

Jasper made a branch "wip/new-model" which will ensure that we only have
one version number per build, among other things.  Sending to the list
for general interest.

So far, I've cherry-picked a few patches to master.  The first problem I
hit was with:
https://git.gnome.org/browse/gnome-ostree/commit/?h=wip/new-model&id=462792017d809d2d4d470360ab307cc07064999b
build.js was actually using imports.JsonDB.  Fixed that and pushed.  BTW
did you use any script to do the imports analysis?

I just used C-s in emacs to look for results, so it's not complete. Sorry about that, though :(

 

Next, with:
https://git.gnome.org/browse/gnome-ostree/commit/?h=wip/new-model&id=a95b02202a7b83c001e60098ebd1695380be6922
At the moment the ostree.modules jhbuild moduleset is sticking to GNOME
3.8 branches, and there js185 does *not* have String.endsWith.  We could
monkey patch it in though.

Yeah, OK, I'll drop this one for now, but perhaps a polyfill for older environments (in gjs even?) is a good idea.

 

Merged:
https://git.gnome.org/browse/gnome-ostree/commit/?h=wip/new-model&id=76166e2b823f5cff2a97ba194e98d7484133cd63
https://git.gnome.org/browse/gnome-ostree/commit/?h=wip/new-model&id=89b8b53a61e6ba77b6a099813dcb4e6934d876fa
https://git.gnome.org/browse/gnome-ostree/commit/?h=wip/new-model&id=7c320dac679668fa8d84af4339e4195c3dfbc59a

Can you explain:
https://git.gnome.org/browse/gnome-ostree/commit/?h=wip/new-model&id=52a393dbff696ef88cdf2aae2703ad76ec010793
What symlink was being concurrently modified?

I think at one time I was going to use atomicSymlinkSwap from a forked-off task, so if two tasks tried to symlink at the same time, they could overwrite each other's "current-tmp", but I dropped that, so it's not really needed. Note that I think that the commit is correct anyway (current-tmp is completely bogus), but I dropped it for now.

 

This one is interesting:
https://git.gnome.org/browse/gnome-ostree/commit/?h=wip/new-model&id=4969458107c78ee9fbe20af4f8320ba98e26a70e
One problem though is now multiple queueResolve() will overwrite the
previous one.  So if we get two git push notifications while a resolve
is running, we'll only fetch the second one.

This line is incorrect:
  + let name = matches[i]['name'];
Should be matches[j].  (I really wish JS had a better array iteration
syntax...)

That cleans up the code considerably.

 

- if (this.parameters.fetchAll || this.parameters.fetchComponents.length > 0) {
+ if (this.parameters.fetchAll || componentsToFetch.length > 0) {

You left the "fetchComponents" parameter in the list, but it now does
nothing.  I actually use it from the command line now, like:

$ ostbuild make resolve fetchComponents='["glib"]'

Uh,

        let componentsToFetch = this.parameters.fetchComponents.slice();

 

Am I missing something here, or does componentsToFetch not start out as the fetchComponents list?

Can you just add a componentsToFetch.push.apply(componentsToFetch,
this.parameters.fetchComponents) ?

I'll get to the others in a bit.  I'm hoping to land this weekend the
deployment 2.0 model branches for various git modules.

I've pushed a reworked set of patches for this fixing most of the other issues to wip/new-model.

--
  Jasper

--
  Jasper