Fix polymer build steps so we don't trust existing packages symlinks or folders.

Fixes in polymer build related to 'packages':
 - we don't trust existing packages symlinks or folders.
 - create relative symlinks so subfolders can be valid entrypoints

Fixes for: dartbug.com/13219 dartbug.com/13185

R=jmesserly@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27435

[Siggi Cherem (dart-lang), 2013-09-11 23:54:58]

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
File pkg/polymer/lib/src/barback_runner.dart (left):

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:195: Future _emitFiles(Barback barback,
BarbackOptions options, String emitSubDir) {
this code became _emitTransformedFiles

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:217: return
Future.wait(futures).then((_) {
this code moved to _emitPackagesDir

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
File pkg/polymer/lib/src/barback_runner.dart (right):

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:257: _deleteIfPresent(outPackages);
alternatively, instead of deleting 'packages/' I can just delete the entire
outDir. Any preferences?

[Siggi Cherem (dart-lang), 2013-09-11 23:56:22]

just to clarify what this change does in terms of layout:

before:

web/
  | packages/...
  | index.html
test/
  | packages/... # emitted again
  | test.html

after:

packages/
web/
  | packages -> ../packages/
  | index.html
test/
  | packages -> ../packages/
  | test.html

[Jennifer Messerly, 2013-09-12 02:45:03]

Some suggestions, but LGTM with or without them :)

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
File pkg/polymer/lib/src/barback_runner.dart (left):

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:195: Future _emitFiles(Barback barback,
BarbackOptions options, String emitSubDir) {
On 2013/09/11 23:54:58, Siggi Cherem (dart-lang) wrote:
> this code became _emitTransformedFiles

yeah, nice cleanup! I like how the main Future chain is isolated into
_emitAllFiles

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
File pkg/polymer/lib/src/barback_runner.dart (right):

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:280: void _deleteIfPresent(var
packagesPath) {
nit: "var" doesn't add anything.

you could type this to String if you wanted to?

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:282: if (link.existsSync()) {
I dunno if it's worth worrying about, but every time I see this pattern or use
it myself, it makes me think that technically it isn't safe. Another process
could (in theory) mess up the path right before we touch it again. Ideally we'd
have an API that would allow us to express the intent, which is more like a
transaction.

Anyway there's nothing to really fix here, since it's an issue with the API
you're building on. We could go all crazy with try+catch guards around these
APIs, but I dunno if it's worth the added ugliness.

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:293: var file = new File(packagesPath);
do you actually need to check for all 3 kinds of things?

from a few quick tests:

* new File(path).deleteSync() works on symlinks
* new Directory(path).deleteSync(recursive: true) works on symlinks, and will
not delete contents.
* new Directory(path).deleteSync(recursive: true) works on files too

this makes me think that `new Directory(path).deleteSync(recursive: true)` is
all you need here.

... combining this with the earlier suggestion, perhaps just this?

void _deleteIfPresent(String path) {
  try {
    new Directory(path).deleteSync(recursive: true);
  } on DirectoryException catch (e) {}
}


For the record, I didn't try this on any OS except Linux, so no promises... :)

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:310: Future _copyFile(String inpath,
String outpath) {
this feels like a helper that should exist somewhere...

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/test/run.sh
File pkg/polymer/test/run.sh (right):

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/test/run.sh#ne...
pkg/polymer/test/run.sh:19: CONFIG="-m release -r vm,drt,ff,chrome -c
none,dart2js,dartanalyzer --checked $*"
Nice. My original idea was that DRT approx equal to Chrome, but I think the bots
have proven that theory wrong :). +1 for adding here.

[Siggi Cherem (dart-lang), 2013-09-12 16:53:14]

Thanks!

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
File pkg/polymer/lib/src/barback_runner.dart (right):

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:280: void _deleteIfPresent(var
packagesPath) {
On 2013/09/12 02:45:03, John Messerly wrote:
> nit: "var" doesn't add anything.
> 
> you could type this to String if you wanted to?

thanks, I think I copied it from the other method, which I copied from somewhere
else. Fixed.

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:282: if (link.existsSync()) {
On 2013/09/12 02:45:03, John Messerly wrote:
> I dunno if it's worth worrying about, but every time I see this pattern or use
> it myself, it makes me think that technically it isn't safe. Another process
> could (in theory) mess up the path right before we touch it again. Ideally
we'd
> have an API that would allow us to express the intent, which is more like a
> transaction.
> 
> Anyway there's nothing to really fix here, since it's an issue with the API
> you're building on. We could go all crazy with try+catch guards around these
> APIs, but I dunno if it's worth the added ugliness.

good point. I added the try+catch.

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:293: var file = new File(packagesPath);
On 2013/09/12 02:45:03, John Messerly wrote:
> do you actually need to check for all 3 kinds of things?
> 
> from a few quick tests:
> 
> * new File(path).deleteSync() works on symlinks
> * new Directory(path).deleteSync(recursive: true) works on symlinks, and will
> not delete contents.
> * new Directory(path).deleteSync(recursive: true) works on files too
> 
> this makes me think that `new Directory(path).deleteSync(recursive: true)` is
> all you need here.
> 
> ... combining this with the earlier suggestion, perhaps just this?
> 
> void _deleteIfPresent(String path) {
>   try {
>     new Directory(path).deleteSync(recursive: true);
>   } on DirectoryException catch (e) {}
> }
> 
> 
> For the record, I didn't try this on any OS except Linux, so no promises... :)

I'm a little paranoid on whether this works in Windows. I just found  that pub
has a similar pattern, they basically do the 3 checks as well.

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:310: Future _copyFile(String inpath,
String outpath) {
On 2013/09/12 02:45:03, John Messerly wrote:
> this feels like a helper that should exist somewhere...

Yeah, I couldn't find it anywhere in the repo though

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/test/run.sh
File pkg/polymer/test/run.sh (right):

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/test/run.sh#ne...
pkg/polymer/test/run.sh:19: CONFIG="-m release -r vm,drt,ff,chrome -c
none,dart2js,dartanalyzer --checked $*"
On 2013/09/12 02:45:03, John Messerly wrote:
> Nice. My original idea was that DRT approx equal to Chrome, but I think the
bots
> have proven that theory wrong :). +1 for adding here.

:)

[Siggi Cherem (dart-lang), 2013-09-12 16:53:51]

Committed patchset #3 manually as r27435 (presubmit successful).

[Jennifer Messerly, 2013-09-12 17:53:47]

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
File pkg/polymer/lib/src/barback_runner.dart (right):

https://codereview.chromium.org/23876012/diff/3001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_runner.dart:293: var file = new File(packagesPath);
On 2013/09/12 16:53:14, Siggi Cherem (dart-lang) wrote:
> On 2013/09/12 02:45:03, John Messerly wrote:
> > do you actually need to check for all 3 kinds of things?
> > 
> > from a few quick tests:
> > 
> > * new File(path).deleteSync() works on symlinks
> > * new Directory(path).deleteSync(recursive: true) works on symlinks, and
will
> > not delete contents.
> > * new Directory(path).deleteSync(recursive: true) works on files too
> > 
> > this makes me think that `new Directory(path).deleteSync(recursive: true)`
is
> > all you need here.
> > 
> > ... combining this with the earlier suggestion, perhaps just this?
> > 
> > void _deleteIfPresent(String path) {
> >   try {
> >     new Directory(path).deleteSync(recursive: true);
> >   } on DirectoryException catch (e) {}
> > }
> > 
> > 
> > For the record, I didn't try this on any OS except Linux, so no promises...
:)
> 
> I'm a little paranoid on whether this works in Windows. I just found  that pub
> has a similar pattern, they basically do the 3 checks as well.

yeah, I wonder though. Pub can be a tricky place to copy code from; as the first
big thing using dart:io cross platform, they developed a lot of crazy
workarounds, and sometimes the original bug has been fixed for a long time :).


anyway, if the pattern is required, it seems like it should be in a
library/package somewhere. (it's a pet peeve of mine when something simple like
"delete this path if it exists" becomes a 15 line function ;) )