Improve how we handle packages/ HTML imports. This resolves better any valid

Improve how we handle packages/ HTML imports. This improves resolution of URLs containing
"package/" under the web/ directory. Resolution under 'lib' is the same, but it and gives better suggestions if the path looks wrong. This summarizes what changed:

- source file: lib/b.html

  import to "packages/c/d.html" was error, now is error + suggestion
  import to "../packages/c/d.html" was error, now is error + suggestion
  import to "../../packages/c/d.html" was resolved, now is resolved too

- source file: lib/a/b.html

  import to "packages/c/d.html" was error, now is error + suggestion
  import to "../packages/c/d.html" was error, now is error + suggestion
  import to "../../packages/c/d.html" was error, now is error + suggestion
  import to "../../../packages/c/d.html" was resolved, now is resolved too


- source file: web/b.html

  import to "packages/c/d.html" was and continues to be resolved
  import to "../packages/c/d.html" was error, now is resolved

- source file: web/a/b.html

  import to "packages/c/d.html" was error, now is resolved
  import to "../packages/c/d.html" was and continues to be resolved
  import to "../../packages/c/d.html" was and continues to be resolved

Note: polymer-build and development mode are slightly different: polymer-build
detects and issues an error for bad paths inside lib/. These are not detected by
our package-resolution in loader.dart, but as long as users are not creating
symlinks by hand, Dartium should detect if the paths are wrong.

When we switch to start using pub-serve by default, we should be able to go back
to the web-ui style of using "package:" urls directly in HTML.

R=jmesserly@google.com, kustermann@google.com

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

[Siggi Cherem (dart-lang), 2014-01-29 01:28:59]

Martin - I'm making a small change in test_suite to support subdirectories in
polymer tests

John - canonicalization2 and canonicalization3 are negative tests. I had to
create these new test packages because the build process fails if any file under
a package contains errors (even if the file is not used in the actual test).

[kustermann, 2014-01-29 20:48:39]

https://codereview.chromium.org/148913003/diff/60001/tools/testing/dart/test_...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/148913003/diff/60001/tools/testing/dart/test_...
tools/testing/dart/test_suite.dart:1136: } while (!new
File(pubspec).existsSync());
There is already logic to do something similar. Please update your local working
copy. 
See here:
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/tool...

https://codereview.chromium.org/148913003/diff/60001/tools/testing/dart/test_...
tools/testing/dart/test_suite.dart:1137: htmlPath =
'$tempDir/${dir.relativeTo(homeDir)}/$nameNoExt.html';
I don't understand what the purpose of this is. The enqueuing phase will look
recursively for tests (i.e. not  just the files inside pkg/X/test/).

The only thing you change here is "htmlPath". Does the polymer build/deploy
command put the files in a different directory now?

Could you explain why this is necessary?

[Siggi Cherem (dart-lang), 2014-01-29 22:15:07]

https://codereview.chromium.org/148913003/diff/60001/tools/testing/dart/test_...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/148913003/diff/60001/tools/testing/dart/test_...
tools/testing/dart/test_suite.dart:1136: } while (!new
File(pubspec).existsSync());
On 2014/01/29 20:48:40, kustermann wrote:
> There is already logic to do something similar. Please update your local
working
> copy. 
> See here:
>
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/tool...

Thanks. All set.

https://codereview.chromium.org/148913003/diff/60001/tools/testing/dart/test_...
tools/testing/dart/test_suite.dart:1137: htmlPath =
'$tempDir/${dir.relativeTo(homeDir)}/$nameNoExt.html';
On 2014/01/29 20:48:40, kustermann wrote:
> I don't understand what the purpose of this is. The enqueuing phase will look
> recursively for tests (i.e. not  just the files inside pkg/X/test/).
> 
> The only thing you change here is "htmlPath". Does the polymer build/deploy
> command put the files in a different directory now?
> 

Yes and no - it always supported putting the output file in different
directories, but we haven't written tests that exposed this until now.

> Could you explain why this is necessary?

The polymer-build command works similar to pub-build: for a given file it finds
where the package directory lives (finding the pubspec file), and it generates
output that maintains the layout of original package.

There are 2 packages in the repo containing polymer tests. Most tests are in the
polymer package under "pkg/polymer/test/", but we have some other tests under a
test-only package called 'canonicalization'. This package is currently under
"pkg/polymer/example/canonicalization/" (it is practically a nested package, we
could also move that somewhere else if it makes things cleaner).

Up until now, all of tests were placed directly under the 'test/' folder of
their respective packages, so that made every test be OK with the output
described as: "$tmpDir/test/"

In this CL I had to add a test that lives deeper inside the test folder of the
canonicalization package: "test/dir/dev_test", so the changes in this file are
needed to be able to find the generated output (in this case under
"$tmpDir/test/dir/").

https://codereview.chromium.org/148913003/diff/80001/tools/testing/dart/test_...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/148913003/diff/80001/tools/testing/dart/test_...
tools/testing/dart/test_suite.dart:829: static Path _findPubspecYamlFile(Path
filePath) {
no changes here, I just extracted it and made it private

[Jennifer Messerly, 2014-01-29 23:07:16]

lgtm

https://codereview.chromium.org/148913003/diff/80001/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/148913003/diff/80001/pkg/pkg.status#newcode125
pkg/pkg.status:125: polymer/example/canonicalization2: Skip
just curious, would the above line already be enough?

in most cases the dart test filters seem to act more like prefixes ... e.g.
running html/custom works for that folder but also html/custom_element_test.dart

[Siggi Cherem (dart-lang), 2014-01-29 23:10:14]

thx!

https://codereview.chromium.org/148913003/diff/80001/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/148913003/diff/80001/pkg/pkg.status#newcode125
pkg/pkg.status:125: polymer/example/canonicalization2: Skip
On 2014/01/29 23:07:16, John Messerly wrote:
> just curious, would the above line already be enough?
> 
> in most cases the dart test filters seem to act more like prefixes ... e.g.
> running html/custom works for that folder but also
html/custom_element_test.dart

Yeah, I think so too, but I wanted to call these out explicitly.

[kustermann, 2014-01-30 01:41:34]

Testing script changes: lgtm.

Thanks for the explanation.

@ricow: This relative path to polymer pubspec.yaml might break the custom
package-root support that we have/will roll out (i.e. tests will start failing).
Please be aware of this.

https://codereview.chromium.org/148913003/diff/80001/pkg/polymer/example/cano...
File pkg/polymer/example/canonicalization2/pubspec.yaml (right):

https://codereview.chromium.org/148913003/diff/80001/pkg/polymer/example/cano...
pkg/polymer/example/canonicalization2/pubspec.yaml:4: path: ../../
The canonicalization2 package is using polymer and that should be it. 

You should not use any relative paths here. Why can't you just use 'polymer' as
a dependency?

https://codereview.chromium.org/148913003/diff/80001/pkg/polymer/example/cano...
File pkg/polymer/example/canonicalization3/pubspec.yaml (right):

https://codereview.chromium.org/148913003/diff/80001/pkg/polymer/example/cano...
pkg/polymer/example/canonicalization3/pubspec.yaml:4: path: ../../
ditto.

https://codereview.chromium.org/148913003/diff/80001/tools/testing/dart/test_...
File tools/testing/dart/test_controller.js (right):

https://codereview.chromium.org/148913003/diff/80001/tools/testing/dart/test_...
tools/testing/dart/test_controller.js:237: // inlined error handler in the HTML
files that wrap DRT tests, and manually in
"in the HTML files that wrap DRT tests" -> "in the generated HTML files" ? 

I don't think this is DRT specific.

[Siggi Cherem (dart-lang), 2014-01-30 02:25:00]

https://codereview.chromium.org/148913003/diff/80001/pkg/polymer/example/cano...
File pkg/polymer/example/canonicalization2/pubspec.yaml (right):

https://codereview.chromium.org/148913003/diff/80001/pkg/polymer/example/cano...
pkg/polymer/example/canonicalization2/pubspec.yaml:4: path: ../../
On 2014/01/30 01:41:34, kustermann wrote:
> The canonicalization2 package is using polymer and that should be it. 
> 
> You should not use any relative paths here. 

I'm not sure why? These packages are apps used for testing only and are never
intended to be published.

> Why can't you just use 'polymer' as a dependency?

I guess you mean 'polymer: any'?

We often accumulate several changes before we publish them. I'm using a relative
path to capture more accurately when a test depends on unpublished features.

That being said, I'm not sure how are these version dependencies used by the
bots. Are the bots going to test with pub's hosted version of polymer, or are we
overriding the dependency to use the existing sources from the repo?

In the case of 'canonicalization*', some of these tests will fail if we run them
with the currently published version of polymer (0.9.4). I plan to release 0.9.5
soon afterwards (I just updated this CL to include a version bump on polymer),
but in the meantime we need to run them against the repo sources.

If the bots ignore/override the dependency version, then I can change the
dependency here to say:
  polymer: ">=0.9.5"

https://codereview.chromium.org/148913003/diff/80001/tools/testing/dart/test_...
File tools/testing/dart/test_controller.js (right):

https://codereview.chromium.org/148913003/diff/80001/tools/testing/dart/test_...
tools/testing/dart/test_controller.js:237: // inlined error handler in the HTML
files that wrap DRT tests, and manually in
On 2014/01/30 01:41:34, kustermann wrote:
> "in the HTML files that wrap DRT tests" -> "in the generated HTML files" ? 
> 
> I don't think this is DRT specific.

Done.

[Siggi Cherem (dart-lang), 2014-01-30 20:51:56]

Committed patchset #4 manually as r32170 (presubmit successful).

[kustermann, 2014-01-31 19:15:09]

https://codereview.chromium.org/148913003/diff/80001/pkg/polymer/example/cano...
File pkg/polymer/example/canonicalization2/pubspec.yaml (right):

https://codereview.chromium.org/148913003/diff/80001/pkg/polymer/example/cano...
pkg/polymer/example/canonicalization2/pubspec.yaml:4: path: ../../
On 2014/01/30 02:25:00, Siggi Cherem (dart-lang) wrote:
> On 2014/01/30 01:41:34, kustermann wrote:
> > The canonicalization2 package is using polymer and that should be it. 
> > 
> > You should not use any relative paths here. 
> 
> I'm not sure why? These packages are apps used for testing only and are never
> intended to be published.
> 
> > Why can't you just use 'polymer' as a dependency?
> 
> I guess you mean 'polymer: any'?
> 
> We often accumulate several changes before we publish them. I'm using a
relative
> path to capture more accurately when a test depends on unpublished features.
> 
> That being said, I'm not sure how are these version dependencies used by the
> bots. Are the bots going to test with pub's hosted version of polymer, or are
we
> overriding the dependency to use the existing sources from the repo?
> 
> In the case of 'canonicalization*', some of these tests will fail if we run
them
> with the currently published version of polymer (0.9.4). I plan to release
0.9.5
> soon afterwards (I just updated this CL to include a version bump on polymer),
> but in the meantime we need to run them against the repo sources.
> 
> If the bots ignore/override the dependency version, then I can change the
> dependency here to say:
>   polymer: ">=0.9.5"

It will be an issue once all our bots will use "real" package-roots (currently
we stupidly create symlinks int out/ReleaseIA32/packages ...):

It was requested that we test all our packages/samples with packages from the
repository AND with packages from pub.dartlang.org. We've created infrastructure
for doing this:
- for every test we see if it's inside a package (canonicalization2 in this
case)
- we will then copy that pubspec.yaml into a temporary place and call 'pub get'
to fetch the dependencies (**)
- we will use the packages directory created by 'pub get' as a package-root for
the test (i.e. we pass it to dart2js/vm/...)

(**) When using public packages we don't modify the pubspec.yaml file. When
using the packages in the repository, we use "dependency overrides" for the
packages we have in the repo.

Now you probably see the issue: If you use a relative path to polymer, and we
copy the pubspec.yaml file to another place and call 'pub get', the relative
path is wrong. I'm not sure if we should have relative paths at all, or we
should special case this situation ....

Note that even though the infrastructure is there (./tools/test.py
--use-public-packages / ./tools/test.py --use-repository-packages), it's not
enabled on the bots yet.

For further discussions please get in touch with rico :)