Teach dart_package to understand the packages/ subdirectory

Teach dart_package to understand the packages/ subdirectory

This CL allows dart_packages to use pubspec.yaml files:
- It adds a script that outputs a list of the files that exist in a target
  directory's "packages/" subdirectory.
- It adds a "uses_pub" var that a dart_package can set to indicate that it is
  using pub. If that variable is set, the dart_package will have a new
  dependency that repackages the files that were pulled into the packages/
  subdirectory into a dartzip file.
- It adds a script that runs "pub get" in all directories that have a checked-
  in pubspec.yaml file. This script can be run as a hook in "gclient sync" to
  avoid the need to manually run "pub get".

This CL also adds the glient sync hook to the Mojo repo, changes
//mojo/dart/apptest/BUILD.gn to use "uses_pub", and eliminates
//third_party/dart-packages.

R=tonyg@chromium.org, zra@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/a5e74398fe1a1844bf41e12f4c8d4ffc050f346b

[blundell, 2015-04-08 10:29:48]

blundell@chromium.org changed reviewers:
+ tonyg@chromium.org, zra@google.com

[tonyg, 2015-04-08 13:52:38]

This is awesome! A few comments, but all minor stuff. I'm really looking forward
to this.

https://codereview.chromium.org/1063233004/diff/40001/build/module_args/mojo.gni
File build/module_args/mojo.gni (right):

https://codereview.chromium.org/1063233004/diff/40001/build/module_args/mojo....
build/module_args/mojo.gni:8: # This variable should point to the parent
directory of the Dart SDK. It can be
It's not really clear to me why this should point to the parent instead of
"//third_party/dart-sdk/dart-sdk/"

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
File mojo/dart/apptest/pubspec.lock (right):

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
mojo/dart/apptest/pubspec.lock:1: # Generated by pub
It seems worth documenting the process of updating the locked versions in a
README.md somewhere. Is it something like, rm this file, manually pub get, then
check in the new one? Or is there a better way?

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
File mojo/dart/apptest/pubspec.yaml (right):

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
mojo/dart/apptest/pubspec.yaml:1: name: mojo_apptest
Not necessary in this patch, but now that this is a real pub, I think it'd be
appropriate to give it a description and everything and actually publish it like
we publish the mojo and sky pubs. Then we can remove that hack that uploads a
specific version to cloud storage and other repos can just depend on the pub
itself.

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
mojo/dart/apptest/pubspec.yaml:4: unittest: any
I think think this'd be more future-proof if we went with the recommended
pattern, ">=0.11.5+4 <0.12.0", as per:
https://pub.dartlang.org/packages/unittest

Although, maybe that complicates the README.md because if someone wants to
switch to v0.12, they edit this file and the lock?

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/dart/rules.gni
File mojo/public/dart/rules.gni (right):

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/dart/rules....
mojo/public/dart/rules.gni:25: inputs = exec_script(pub_get_script,
Curious about a couple of things:
1. Does pub get try to run every time, or does this rule know not to try if
there's a stamp file or something?
2. What does it look like to the user when they're offline and pub get has to
hit the network? I'd like to make sure the message is really clear.

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/tools/dart_...
File mojo/public/tools/dart_pub_get.py (right):

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/tools/dart_...
mojo/public/tools/dart_pub_get.py:6: # This script runs "pub get" on a given
directory and outputs the filenames of
This looks like a module-level docstring, """Lorem ipsum""", rather than a
comment, # Lorem ipsum.

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/tools/dart_...
mojo/public/tools/dart_pub_get.py:7: # the files that are thus pulled in
relative to that given directory.
I think a link to documentation would be helpful in this comment.
https://www.dartlang.org/tools/pub/get-started.html might be the most canonical?

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/tools/dart_...
mojo/public/tools/dart_pub_get.py:19: dart_sdk_root = args[0]
This is a nit, but I'd recommend using argparse instead of manually pulling the
args. Then you can replace most of the usage comment above with structured usage
which is both obvious in code and from ./dart_pub_get.py --help.

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/tools/dart_...
mojo/public/tools/dart_pub_get.py:22: os.path.join(os.getcwd(), dart_sdk_root,
"dart-sdk/bin/pub")
I could be mistaken, but I don't think os.getcwd() is what you actually want
here. That means this path'd be incorrect if this script was invoked from src/.

Instead, I think you want to grab the absolute path of this file and add the
appropriate number of os.pardir's to get back to src. Something like:
os.path.join(os.path.realpath(__file__), os.pardir, os.pardir, os.pardir,
dart_sdk_root, "")

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/tools/dart_...
mojo/public/tools/dart_pub_get.py:27: subprocess.check_output(cmd, shell=False,
stderr=subprocess.STDOUT)
Cleaner to pass cwd=target_dir rather than than doing an os.chdir on the line
above.

[zra, 2015-04-08 14:54:35]

zra@google.com changed reviewers:
+ jamesr@chromium.org, johnmccutchan@google.com

[zra, 2015-04-08 14:54:36]

+jamesr, who had reservations about hitting the network during build.
+johnmccutchan, who did something similar for the Dart tree

John can elaborate, but the strategy we use in the Dart tree for a similar
situation is the following:

'gclient sync' pulls the necessary Dart pub packages into third_party/

During the build, to make a Dart pub package, we use 'pub get --offline'. This
requires the pubspec.yaml to include dependency overrides like here for the
Observatory:

https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/runt...

On the other hand, allowing dart_package to hit the network could make life a
lot simpler for Mojo SDK consumers. They could write a pubspec.yaml like a
normal Dart developer, then. So, maybe it makes sense to have two templates:
dart_package and dart_package_offline, where dart_package is as in this CL, and
dart_package_offline uses the scheme above. Within the Mojo tree, we'd then only
use dart_package_offline, but SDK consumers would be free to choose the one they
like.

Another concern is that pub downloads packages to ~/.pub-cache. I don't know how
the builders work, but if you have multiple builds going on at once, they could
all be trying read/write the same files. pub can be made to use a different
directory for its cache by setting the PUB_CACHE environment variable. I'd
recommend setting this to somewhere in the build output directory.

[Cutch, 2015-04-08 16:16:10]

On 2015/04/08 14:54:36, zra wrote:
> +jamesr, who had reservations about hitting the network during build.
> +johnmccutchan, who did something similar for the Dart tree
> 
> John can elaborate, but the strategy we use in the Dart tree for a similar
> situation is the following:
> 
> 'gclient sync' pulls the necessary Dart pub packages into third_party/
> 
> During the build, to make a Dart pub package, we use 'pub get --offline'. This
> requires the pubspec.yaml to include dependency overrides like here for the
> Observatory:
> 
>
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/runt...
> 
> On the other hand, allowing dart_package to hit the network could make life a
> lot simpler for Mojo SDK consumers. They could write a pubspec.yaml like a
> normal Dart developer, then. So, maybe it makes sense to have two templates:
> dart_package and dart_package_offline, where dart_package is as in this CL,
and
> dart_package_offline uses the scheme above. Within the Mojo tree, we'd then
only
> use dart_package_offline, but SDK consumers would be free to choose the one
they
> like.
> 
> Another concern is that pub downloads packages to ~/.pub-cache. I don't know
how
> the builders work, but if you have multiple builds going on at once, they
could
> all be trying read/write the same files. pub can be made to use a different
> directory for its cache by setting the PUB_CACHE environment variable. I'd
> recommend setting this to somewhere in the build output directory.

What we do in the Dart tree for Observatory can be broken into two stages:

stage 1: performed by the maintainer (me)

1) run `pub get` to download all the packages (including transitive
dependencies).
2) Perform a deep copy of packages directory and check that into
third_party/observatory_pub_packages.
3) Bump DEPS for third_party/observatory_pub_packages.

stage 2: performed at build time

1) Rewrite pubspec.yaml to have a dependency_overrides statement for each
dependency (explicitly listing all transitive dependencies)
2) Run `pub get --offline` (sets up packages symlinks
third_party/observatory_pub_packages/$FOO)

Stage 1 is a manual process (assisted by some shell scripts) and done only when
dependencies need to be upgraded.

Stage 2 never hits the network.

This isn't ideal but it works.

[blundell, 2015-04-08 16:19:54]

On 2015/04/08 16:16:10, Cutch wrote:
> On 2015/04/08 14:54:36, zra wrote:
> > +jamesr, who had reservations about hitting the network during build.
> > +johnmccutchan, who did something similar for the Dart tree
> > 
> > John can elaborate, but the strategy we use in the Dart tree for a similar
> > situation is the following:
> > 
> > 'gclient sync' pulls the necessary Dart pub packages into third_party/
> > 
> > During the build, to make a Dart pub package, we use 'pub get --offline'.
This
> > requires the pubspec.yaml to include dependency overrides like here for the
> > Observatory:
> > 
> >
>
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/runt...
> > 
> > On the other hand, allowing dart_package to hit the network could make life
a
> > lot simpler for Mojo SDK consumers. They could write a pubspec.yaml like a
> > normal Dart developer, then. So, maybe it makes sense to have two templates:
> > dart_package and dart_package_offline, where dart_package is as in this CL,
> and
> > dart_package_offline uses the scheme above. Within the Mojo tree, we'd then
> only
> > use dart_package_offline, but SDK consumers would be free to choose the one
> they
> > like.
> > 
> > Another concern is that pub downloads packages to ~/.pub-cache. I don't know
> how
> > the builders work, but if you have multiple builds going on at once, they
> could
> > all be trying read/write the same files. pub can be made to use a different
> > directory for its cache by setting the PUB_CACHE environment variable. I'd
> > recommend setting this to somewhere in the build output directory.
> 
> What we do in the Dart tree for Observatory can be broken into two stages:
> 
> stage 1: performed by the maintainer (me)
> 
> 1) run `pub get` to download all the packages (including transitive
> dependencies).
> 2) Perform a deep copy of packages directory and check that into
> third_party/observatory_pub_packages.
> 3) Bump DEPS for third_party/observatory_pub_packages.
> 
> stage 2: performed at build time
> 
> 1) Rewrite pubspec.yaml to have a dependency_overrides statement for each
> dependency (explicitly listing all transitive dependencies)
> 2) Run `pub get --offline` (sets up packages symlinks
> third_party/observatory_pub_packages/$FOO)
> 
> Stage 1 is a manual process (assisted by some shell scripts) and done only
when
> dependencies need to be upgraded.
> 
> Stage 2 never hits the network.
> 
> This isn't ideal but it works.

Thanks for the info, and thanks as well for the reminder about not hitting the
network during the build!

I'll try out something like this:
- Have a script that finds all the checked-in pubspec.yaml files in the tree and
runs "pub get" in the corresponding directories.
- Add a hook to "gclient sync" to run this script.
- Modify the build step to not run "pub get" before it spits out the contents of
the "packages/" subdir.

This will avoid hitting the network during the build while keeping developer
flow more-or-less the same.

[blundell, 2015-04-09 11:53:42]

All comments addressed (including putting .pub-cache inside the repo and not
hitting the network during the build).

https://codereview.chromium.org/1063233004/diff/40001/build/module_args/mojo.gni
File build/module_args/mojo.gni (right):

https://codereview.chromium.org/1063233004/diff/40001/build/module_args/mojo....
build/module_args/mojo.gni:8: # This variable should point to the parent
directory of the Dart SDK. It can be
On 2015/04/08 13:52:37, tonyg wrote:
> It's not really clear to me why this should point to the parent instead of
> "//third_party/dart-sdk/dart-sdk/"

Just for consistency with mojo_sdk_root :).

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
File mojo/dart/apptest/pubspec.lock (right):

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
mojo/dart/apptest/pubspec.lock:1: # Generated by pub
On 2015/04/08 13:52:37, tonyg wrote:
> It seems worth documenting the process of updating the locked versions in a
> README.md somewhere. Is it something like, rm this file, manually pub get,
then
> check in the new one? Or is there a better way?

You update pubspec.yaml and then run "pub upgrade". Do you see a good place to
document this?

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
File mojo/dart/apptest/pubspec.yaml (right):

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
mojo/dart/apptest/pubspec.yaml:1: name: mojo_apptest
On 2015/04/08 13:52:37, tonyg wrote:
> Not necessary in this patch, but now that this is a real pub, I think it'd be
> appropriate to give it a description and everything and actually publish it
like
> we publish the mojo and sky pubs. Then we can remove that hack that uploads a
> specific version to cloud storage and other repos can just depend on the pub
> itself.

It's a half-man half-monster now, because it also has dependencies specified in
the BUILD.gn file (which will generally be the case for dart_packages going
forward). Hmmm...

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
mojo/dart/apptest/pubspec.yaml:4: unittest: any
On 2015/04/08 13:52:37, tonyg wrote:
> I think think this'd be more future-proof if we went with the recommended
> pattern, ">=0.11.5+4 <0.12.0", as per:
> https://pub.dartlang.org/packages/unittest
> 
> Although, maybe that complicates the README.md because if someone wants to
> switch to v0.12, they edit this file and the lock?

Done.

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/dart/rules.gni
File mojo/public/dart/rules.gni (right):

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/dart/rules....
mojo/public/dart/rules.gni:25: inputs = exec_script(pub_get_script,
On 2015/04/08 13:52:38, tonyg wrote:
> Curious about a couple of things:
> 1. Does pub get try to run every time, or does this rule know not to try if
> there's a stamp file or something?

The inputs to the inner script are (1) the script itself [by default] and (2)
the target dir's pubspec.lock file [specified in line 31]. So the script will
re-run if either it changes or the pubspec.lock file changes.

> 2. What does it look like to the user when they're offline and pub get has to
> hit the network? I'd like to make sure the message is really clear.

Fixed this by having "pub get" run at "gclient sync" time.

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/tools/dart_...
File mojo/public/tools/dart_pub_get.py (right):

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/tools/dart_...
mojo/public/tools/dart_pub_get.py:6: # This script runs "pub get" on a given
directory and outputs the filenames of
On 2015/04/08 13:52:38, tonyg wrote:
> This looks like a module-level docstring, """Lorem ipsum""", rather than a
> comment, # Lorem ipsum.

Done.

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/tools/dart_...
mojo/public/tools/dart_pub_get.py:7: # the files that are thus pulled in
relative to that given directory.
On 2015/04/08 13:52:38, tonyg wrote:
> I think a link to documentation would be helpful in this comment.
> https://www.dartlang.org/tools/pub/get-started.html might be the most
canonical?

Done.

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/tools/dart_...
mojo/public/tools/dart_pub_get.py:19: dart_sdk_root = args[0]
On 2015/04/08 13:52:38, tonyg wrote:
> This is a nit, but I'd recommend using argparse instead of manually pulling
the
> args. Then you can replace most of the usage comment above with structured
usage
> which is both obvious in code and from ./dart_pub_get.py --help.

Done.

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/tools/dart_...
mojo/public/tools/dart_pub_get.py:22: os.path.join(os.getcwd(), dart_sdk_root,
"dart-sdk/bin/pub")
On 2015/04/08 13:52:38, tonyg wrote:
> I could be mistaken, but I don't think os.getcwd() is what you actually want
> here. That means this path'd be incorrect if this script was invoked from
src/.
> 
> Instead, I think you want to grab the absolute path of this file and add the
> appropriate number of os.pardir's to get back to src. Something like:
> os.path.join(os.path.realpath(__file__), os.pardir, os.pardir, os.pardir,
> dart_sdk_root, "")

Done.

https://codereview.chromium.org/1063233004/diff/40001/mojo/public/tools/dart_...
mojo/public/tools/dart_pub_get.py:27: subprocess.check_output(cmd, shell=False,
stderr=subprocess.STDOUT)
On 2015/04/08 13:52:38, tonyg wrote:
> Cleaner to pass cwd=target_dir rather than than doing an os.chdir on the line
> above.

Done.

[tonyg, 2015-04-09 12:55:57]

lgtm w/ comments

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
File mojo/dart/apptest/pubspec.lock (right):

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
mojo/dart/apptest/pubspec.lock:1: # Generated by pub
On 2015/04/09 11:53:41, blundell wrote:
> On 2015/04/08 13:52:37, tonyg wrote:
> > It seems worth documenting the process of updating the locked versions in a
> > README.md somewhere. Is it something like, rm this file, manually pub get,
> then
> > check in the new one? Or is there a better way?
> 
> You update pubspec.yaml and then run "pub upgrade". Do you see a good place to
> document this?

One idea for a follow-up patch would be to output warnings with instructions
when newer packages are available. Then everyone running gclient sync would know
it is time to attempt a roll.

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
File mojo/dart/apptest/pubspec.yaml (right):

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
mojo/dart/apptest/pubspec.yaml:1: name: mojo_apptest
On 2015/04/09 11:53:41, blundell wrote:
> On 2015/04/08 13:52:37, tonyg wrote:
> > Not necessary in this patch, but now that this is a real pub, I think it'd
be
> > appropriate to give it a description and everything and actually publish it
> like
> > we publish the mojo and sky pubs. Then we can remove that hack that uploads
a
> > specific version to cloud storage and other repos can just depend on the pub
> > itself.
> 
> It's a half-man half-monster now, because it also has dependencies specified
in
> the BUILD.gn file (which will generally be the case for dart_packages going
> forward). Hmmm...

Seems like a good area for further thought in a follow-up. I can definitely
imagine that users of the mojo sdk will want a way to use the build system and
still produce a dart pub.

One hand-wavy idea might be to add a new gn rule, "dart_pub_app". It'd depend on
dart_packages and/or dart_packed_apps and their partially specified
pubspec.yaml. And it'd produce a new directory in the out dir that contains the
full pub to upload (including a pubspec.yaml with additional dependencies if
needed). We'd know we have something workable if it can get rid of
sky/tools/deploy_sdk.py.

Then we'd have a primitive "dart_packaged_app+uses_pub" for pub->gn and we could
add another primitive "dart_pub_app" for gn->pub.

https://codereview.chromium.org/1063233004/diff/60001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1063233004/diff/60001/.gitignore#newcode15
.gitignore:15: pubspec.lock
Wait, this needs to be checked-in in order to have a hermetic build, right?

https://codereview.chromium.org/1063233004/diff/60001/DEPS
File DEPS (right):

https://codereview.chromium.org/1063233004/diff/60001/DEPS#newcode313
DEPS:313: },  {
nit: missing a line break here

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/dart/rules.gni
File mojo/public/dart/rules.gni (right):

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/dart/rules....
mojo/public/dart/rules.gni:14: # Repackage all dependencies pulled in via "pub
get" in # a dartzip file.
nit: extra #

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/dart_...
File mojo/public/tools/dart_list_packages_contents.py (right):

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/dart_...
mojo/public/tools/dart_list_packages_contents.py:1: #!/usr/bin/python
bikeshed: /me wonders whether we should have a mojo/public/tools/dart/ directory
instead of prefixing the names of these scripts.

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/git/d...
File mojo/public/tools/git/dart_pub_get.py (right):

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/git/d...
mojo/public/tools/git/dart_pub_get.py:24: # simultaneous builds in different
repos stomping on each other.
I wouldn't change anything here for now, but it seems like we'd want to file a
dart bug if multiple `pub get`s can't be run in parallel.

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/git/d...
mojo/public/tools/git/dart_pub_get.py:40: cmd = ["git", "ls-files",
"*pubspec.yaml"]
I don't know the syntax here, but it looks like this might match
"foopubspec.yaml". Is there a way to allow arbitrary directories but not file
prefixes. Like maybe "*/pubspec.yaml"?

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/git/d...
mojo/public/tools/git/dart_pub_get.py:54: dest="repository_root",
nit: you can omit the dests when the only change is s/-/_/.

[blundell, 2015-04-09 15:34:37]

Thanks! Will wait for zra@'s review as well (plus if anyone else wants to chime
in)

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
File mojo/dart/apptest/pubspec.yaml (right):

https://codereview.chromium.org/1063233004/diff/40001/mojo/dart/apptest/pubsp...
mojo/dart/apptest/pubspec.yaml:1: name: mojo_apptest
On 2015/04/09 12:55:56, tonyg wrote:
> On 2015/04/09 11:53:41, blundell wrote:
> > On 2015/04/08 13:52:37, tonyg wrote:
> > > Not necessary in this patch, but now that this is a real pub, I think it'd
> be
> > > appropriate to give it a description and everything and actually publish
it
> > like
> > > we publish the mojo and sky pubs. Then we can remove that hack that
uploads
> a
> > > specific version to cloud storage and other repos can just depend on the
pub
> > > itself.
> > 
> > It's a half-man half-monster now, because it also has dependencies specified
> in
> > the BUILD.gn file (which will generally be the case for dart_packages going
> > forward). Hmmm...
> 
> Seems like a good area for further thought in a follow-up. I can definitely
> imagine that users of the mojo sdk will want a way to use the build system and
> still produce a dart pub.
> 
> One hand-wavy idea might be to add a new gn rule, "dart_pub_app". It'd depend
on
> dart_packages and/or dart_packed_apps and their partially specified
> pubspec.yaml. And it'd produce a new directory in the out dir that contains
the
> full pub to upload (including a pubspec.yaml with additional dependencies if
> needed). We'd know we have something workable if it can get rid of
> sky/tools/deploy_sdk.py.
> 
> Then we'd have a primitive "dart_packaged_app+uses_pub" for pub->gn and we
could
> add another primitive "dart_pub_app" for gn->pub.

I don't think it's quite that simple, because the main reason to use BUILD.gn
will be to allow dependencies on mojoms, and I don't think that in general we
should be publishing mojom.dart files.

https://codereview.chromium.org/1063233004/diff/60001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1063233004/diff/60001/.gitignore#newcode15
.gitignore:15: pubspec.lock
On 2015/04/09 12:55:56, tonyg wrote:
> Wait, this needs to be checked-in in order to have a hermetic build, right?

pubspec.lock files get generated for everything with a pubspec.yaml file, but
I'm not sure whether everything wants them (e.g., sky).

https://codereview.chromium.org/1063233004/diff/60001/DEPS
File DEPS (right):

https://codereview.chromium.org/1063233004/diff/60001/DEPS#newcode313
DEPS:313: },  {
On 2015/04/09 12:55:56, tonyg wrote:
> nit: missing a line break here

Done.

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/dart/rules.gni
File mojo/public/dart/rules.gni (right):

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/dart/rules....
mojo/public/dart/rules.gni:14: # Repackage all dependencies pulled in via "pub
get" in # a dartzip file.
On 2015/04/09 12:55:56, tonyg wrote:
> nit: extra #

Done.

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/dart_...
File mojo/public/tools/dart_list_packages_contents.py (right):

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/dart_...
mojo/public/tools/dart_list_packages_contents.py:1: #!/usr/bin/python
On 2015/04/09 12:55:57, tonyg wrote:
> bikeshed: /me wonders whether we should have a mojo/public/tools/dart/
directory
> instead of prefixing the names of these scripts.

Let's see if we come up with any more ;).

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/git/d...
File mojo/public/tools/git/dart_pub_get.py (right):

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/git/d...
mojo/public/tools/git/dart_pub_get.py:24: # simultaneous builds in different
repos stomping on each other.
On 2015/04/09 12:55:57, tonyg wrote:
> I wouldn't change anything here for now, but it seems like we'd want to file a
> dart bug if multiple `pub get`s can't be run in parallel.

This was based on feedback from zra@, fwiw.

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/git/d...
mojo/public/tools/git/dart_pub_get.py:40: cmd = ["git", "ls-files",
"*pubspec.yaml"]
On 2015/04/09 12:55:57, tonyg wrote:
> I don't know the syntax here, but it looks like this might match
> "foopubspec.yaml". Is there a way to allow arbitrary directories but not file
> prefixes. Like maybe "*/pubspec.yaml"?

Done.

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/git/d...
mojo/public/tools/git/dart_pub_get.py:54: dest="repository_root",
On 2015/04/09 12:55:57, tonyg wrote:
> nit: you can omit the dests when the only change is s/-/_/.

Done.

[tonyg, 2015-04-09 15:44:20]

https://codereview.chromium.org/1063233004/diff/60001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1063233004/diff/60001/.gitignore#newcode15
.gitignore:15: pubspec.lock
On 2015/04/09 15:34:36, blundell wrote:
> On 2015/04/09 12:55:56, tonyg wrote:
> > Wait, this needs to be checked-in in order to have a hermetic build, right?
> 
> pubspec.lock files get generated for everything with a pubspec.yaml file, but
> I'm not sure whether everything wants them (e.g., sky).

If sky is a special case, then I think we should only ignore it. The safe (and
hopefully common) case is that the lock file is checked in so that builds are
hermetic and deterministic.

[zra, 2015-04-09 16:20:48]

https://codereview.chromium.org/1063233004/diff/60001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1063233004/diff/60001/.gitignore#newcode15
.gitignore:15: pubspec.lock
On 2015/04/09 15:44:20, tonyg wrote:
> On 2015/04/09 15:34:36, blundell wrote:
> > On 2015/04/09 12:55:56, tonyg wrote:
> > > Wait, this needs to be checked-in in order to have a hermetic build,
right?
> > 
> > pubspec.lock files get generated for everything with a pubspec.yaml file,
but
> > I'm not sure whether everything wants them (e.g., sky).
> 
> If sky is a special case, then I think we should only ignore it. The safe (and
> hopefully common) case is that the lock file is checked in so that builds are
> hermetic and deterministic.

If these aren't nailed down, pub during gclient sync is free to pull down a
different version, e.g. when a dependency is for 'any' version. I'd recommend
checking these in.

https://codereview.chromium.org/1063233004/diff/60001/DEPS
File DEPS (right):

https://codereview.chromium.org/1063233004/diff/60001/DEPS#newcode308
DEPS:308: 'src/mojo/public/tools/git/dart_pub_get.py',
There is also a dart_analyze.py, so putting these in //mojo/public/tools/dart/
could make sense. I think not needed for this CL.

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/git/d...
File mojo/public/tools/git/dart_pub_get.py (right):

https://codereview.chromium.org/1063233004/diff/60001/mojo/public/tools/git/d...
mojo/public/tools/git/dart_pub_get.py:24: # simultaneous builds in different
repos stomping on each other.
On 2015/04/09 15:34:36, blundell wrote:
> On 2015/04/09 12:55:57, tonyg wrote:
> > I wouldn't change anything here for now, but it seems like we'd want to file
a
> > dart bug if multiple `pub get`s can't be run in parallel.
> 
> This was based on feedback from zra@, fwiw.

For better or worse, pub was envisioned as a developer tool, not really intended
to run on a builder. By default the pub cache goes under ~/.pub-cache. If there
are multiple pubs running in parallel they could race on reads/writes under that
directory. Since this isn't a use case that pub was originally designed for,
using the PUB_CACHE environment variable is the recommended work-around.

https://codereview.chromium.org/1063233004/diff/80001/mojo/public/dart/rules.gni
File mojo/public/dart/rules.gni (right):

https://codereview.chromium.org/1063233004/diff/80001/mojo/public/dart/rules....
mojo/public/dart/rules.gni:7: import("//build/module_args/mojo.gni")
I think you can't have // in gni files under //mojo/public/. Doesn't the
presubmit script complain?

[jamesr, 2015-04-09 20:04:04]

We definitely need to have checked-in lockfiles for any dependencies fetched by
'pub get'

https://codereview.chromium.org/1063233004/diff/80001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1063233004/diff/80001/.gitignore#newcode11
.gitignore:11: .pub-cache/
can we put this in a more obviously named directory (preferably not a hidden
one)?

https://codereview.chromium.org/1063233004/diff/80001/.gitignore#newcode15
.gitignore:15: pubspec.lock
hmm, why would this lockfile be ignored?

https://codereview.chromium.org/1063233004/diff/80001/.gitignore#newcode20
.gitignore:20: packages/
could this be named more obviously as well? dart_packages/ or the like?

https://codereview.chromium.org/1063233004/diff/80001/mojo/public/dart/rules.gni
File mojo/public/dart/rules.gni (right):

https://codereview.chromium.org/1063233004/diff/80001/mojo/public/dart/rules....
mojo/public/dart/rules.gni:7: import("//build/module_args/mojo.gni")
On 2015/04/09 16:20:48, zra wrote:
> I think you can't have // in gni files under //mojo/public/. Doesn't the
> presubmit script complain?

//build/module_args/mojo.gni is specifically allowed to let users of the SDK
configure build paths and th elike

[blundell, 2015-04-10 10:06:49]

All comments addressed. The issue was that //sky has pubspec.yaml files but
they're not intended for usage in-place in the repo AFAICT. Changed to allow
specifying dirs to ignore (i.e., not run "pub get" in) as an optional argument
to dart_pub_get.py.

https://codereview.chromium.org/1063233004/diff/80001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1063233004/diff/80001/.gitignore#newcode11
.gitignore:11: .pub-cache/
On 2015/04/09 20:04:04, jamesr wrote:
> can we put this in a more obviously named directory (preferably not a hidden
> one)?

Done.

https://codereview.chromium.org/1063233004/diff/80001/.gitignore#newcode15
.gitignore:15: pubspec.lock
On 2015/04/09 20:04:04, jamesr wrote:
> hmm, why would this lockfile be ignored?

Done.

https://codereview.chromium.org/1063233004/diff/80001/.gitignore#newcode20
.gitignore:20: packages/
On 2015/04/09 20:04:04, jamesr wrote:
> could this be named more obviously as well? dart_packages/ or the like?

The "packages/" name is a Dart convention that there's no way to change AFAIK.

[tonyg, 2015-04-10 13:39:20]

lgtm

https://codereview.chromium.org/1063233004/diff/100001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1063233004/diff/100001/.gitignore#newcode18
.gitignore:18: packages/
Alphabetization gets a little wonky around this line.

https://codereview.chromium.org/1063233004/diff/100001/DEPS
File DEPS (right):

https://codereview.chromium.org/1063233004/diff/100001/DEPS#newcode314
DEPS:314: '--dirs-to-ignore', 'sky/',
Kind of feels like there should be a bug to remove this. Or maybe just link to
#72 or #77?

[zra, 2015-04-10 15:04:10]

lgtm

https://codereview.chromium.org/1063233004/diff/80001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1063233004/diff/80001/.gitignore#newcode20
.gitignore:20: packages/
On 2015/04/10 10:06:49, blundell wrote:
> On 2015/04/09 20:04:04, jamesr wrote:
> > could this be named more obviously as well? dart_packages/ or the like?
> 
> The "packages/" name is a Dart convention that there's no way to change AFAIK.

Correct. pub is hardcoded to use a packages/ directory.

[blundell, 2015-04-13 14:23:29]

https://codereview.chromium.org/1063233004/diff/100001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1063233004/diff/100001/.gitignore#newcode18
.gitignore:18: packages/
On 2015/04/10 13:39:20, tonyg wrote:
> Alphabetization gets a little wonky around this line.

Done.

https://codereview.chromium.org/1063233004/diff/100001/DEPS
File DEPS (right):

https://codereview.chromium.org/1063233004/diff/100001/DEPS#newcode314
DEPS:314: '--dirs-to-ignore', 'sky/',
On 2015/04/10 13:39:20, tonyg wrote:
> Kind of feels like there should be a bug to remove this. Or maybe just link to
> #72 or #77?

I'm not sure what the long-term vision is there; going to leave this alone for
now.

[blundell, 2015-04-13 14:32:22]

Committed patchset #8 (id:140001) manually as
a5e74398fe1a1844bf41e12f4c8d4ffc050f346b (presubmit successful).