Expand readable assets for non-root packages, make them configurable #2851

simolus3 · 2020-10-02T16:18:40Z

Instead of throwing an InvalidInputException for non-lib, non-root packages, we now allow the following files by default:

lib/**
bin/**
LICENSE
README*
pubspec.yaml

Each package can expose additional globs with the additional_public_assets build option. I opted to not support exluding assets because

excluding lib/ might lead to unexpected behavior
that package's default target includes additional_public_assets by default. If there were two excludes, it's not clear in which priority they should interfere with each other

In the root package, all files are still allowed of course. I moved the check to throw an InvalidInputException down to the SingleStepReader. Computing the globs of available sources per package now happens in the TargetGraph since that's where the build config is read. That feels a bit unclean since the target graph now decides which assets are visible for a package, please let me know if I should move that logic to the PackageGraph or elsewhere.

Closes #2819. I also wrote an example to crawl licenses across dependencies.

jakemac53

Will take a longer look on Monday, nothing major stands out though.

I think the TargetGraph is the correct place to manage this - that manages what the actual visible individual files are for the package already effectively.

build/lib/src/asset/exceptions.dart

simolus3

There's a problem with the new approach and placeholder nodes like test/$test$ that are added to every package.
When we call runBuilder here, we don't set the rootPackage argument. This causes the BuildStepImpl to believe that the primary input is in fact the root package. Is this a bug? I'm getting test failures when a TestBuilder (applied to multiple packages) calls canRead('nonRootPackage|test/$test$'), which is now considered an invalid input.

So what do we do to fix this? I've considered

disabling the check on placeholder nodes since they can't be read anyways
Not running a builder on primary inputs that are now invalid
Making test/$test$ public by default

To me, option 2 sounds the most reasonable, but it might be considered breaking since builders applied to all packages would no longer run for some placeholder assets. What do you think?

_test_common/lib/test_phases.dart

docs/build_yaml_format.md

jakemac53

There's a problem with the new approach and placeholder nodes like test/$test$ that are added to every package.
When we call runBuilder here, we don't set the rootPackage argument. This causes the BuildStepImpl to believe that the primary input is in fact the root package. Is this a bug? I'm getting test failures when a TestBuilder (applied to multiple packages) calls canRead('nonRootPackage|test/$test$'), which is now considered an invalid input.

So what do we do to fix this? I've considered

disabling the check on placeholder nodes since they can't be read anyways

Not running a builder on primary inputs that are now invalid

Making test/$test$ public by default

To me, option 2 sounds the most reasonable, but it might be considered breaking since builders applied to all packages would no longer run for some placeholder assets. What do you think?

I am not quite fully understanding the issue here - it does seem like a bug that we are not passing the rootPackage argument through though. Does doing that fix the problem?

_test_common/lib/test_phases.dart

build_runner_core/lib/src/package_graph/target_graph.dart

build_runner_core/pubspec.yaml

build_runner_core/test/package_graph/target_graph_test.dart

docs/build_yaml_format.md

jakemac53 · 2020-10-05T16:58:47Z

build/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 1.6.0-dev


Bumping this to 1.6.0 means many other packages have to be republished as well - and the last time I tried to do that I ran into a lot of issues (it is difficult to validate version solves and things with so many intertwined deps).

I am not sure I would have time to go through this whole process again soon I already lost several days of time to the 1.5.x release :(.

That's unfortunate but understandable. I changed the version to 1.5.1.

One thing I wanted to confirm here - if people get this latest version of build but not the latest build_runner_core (which would now be possible) - they will still get an exception when trying to read files outside of lib right? It would just be an AssetNotFoundException instead of an InvalidInputException?

Yes exactly. In the old build_runner_core version, the non-lib files aren't considered by the AssetTracker at all. So one would definitely get an AssetNotFoundException.

natebosch · 2020-10-05T21:20:21Z

cc @natebosch I think might have something he uses here? I don't have a tool I use :/

I do, unfortunately it is internal only. I can fix it up before we merge.

simolus3 · 2020-10-06T08:01:48Z

I am not quite fully understanding the issue here

Consider this test for example. We'd invoke the TestBuilder on a|test/$test, b|test/$test and $sdk$|test/$test (among others). However, only a|test/$test$ is visible in the build and TestBuilder calls buildStep.canRead(buildStep.inputId), so we now get an InvalidInputException on the other two invocations.

If we only passed the rootPackage argument we'd get the same error. Arguably, that test should have failed before too since the inputs have always been invalid. Since those builders invocations are useless, I added an additional check to _matchingPrimaryInputs so that we only run the builder if their primary input is visible in the build. That alone should fix the problem, but I also pass the rootPackage argument to avoid potential other inconsistencies.

I brought this up because I'm not sure if we want to consider this a bugfix or a breaking change. Passing the rootPackage argument also means that findAssets will now run on the root package instead of the package enclosing the primary input.

jakemac53 · 2020-10-06T15:42:36Z

However, only a|test/$test$ is visible in the build and TestBuilder calls buildStep.canRead(buildStep.inputId), so we now get an InvalidInputException on the other two invocations.

Ah - I think the issue here then is that canRead should not throw in this case. It should return false. This api should never throw, as it is the way you ask if it is possible to read some file safely.

jakemac53 · 2020-10-06T15:43:14Z

Passing the rootPackage argument also means that findAssets will now run on the root package instead of the package enclosing the primary input.

Ya we definitely wouldn't want this behavior.

simolus3 · 2020-10-06T18:32:28Z

I think the issue here then is that canRead should not throw in this case

Note that canRead called _checkInput before, so it did throw for invalid inputs in the past. It just missed some (most?) cases due to the rootPackage argument not being set. I can remove that check for canRead now if you want, but I want to make sure that's the behavior we want here.

Passing the rootPackage argument also means that findAssets will now run on the root package instead of the package enclosing the primary input.

Ya we definitely wouldn't want this behavior.

My initial statement was inaccurate, we actually pass inputId?.package ?? rootPackage if we have a MultiPackageAssetReader. However, inputId is guaranteed to be non-null if the BuildStepImpl was created through runBuilder and that would be the last usage of rootPackage, so should I just remove that field and deprecate the parameter?

jakemac53 · 2020-10-08T14:43:52Z

Note that canRead called _checkInput before, so it did throw for invalid inputs in the past. It just missed some (most?) cases due to the rootPackage argument not being set. I can remove that check for canRead now if you want, but I want to make sure that's the behavior we want here.

Oh interesting, I don't think this behavior was intentional - cc @natebosch can you think of why that would have been intentional?

My initial statement was inaccurate, we actually pass inputId?.package ?? rootPackage if we have a MultiPackageAssetReader.

I am a bit confused about this part, where do you see this code?

However, inputId is guaranteed to be non-null if the BuildStepImpl was created through runBuilder and that would be the last usage of rootPackage, so should I just remove that field and deprecate the parameter?

Yes it sounds to me like the rootPackage param on runBuilder should be deprecated - I don't see us passing that anywhere.

simolus3 · 2020-10-08T18:21:40Z

I am a bit confused about this part, where do you see this code?

Sorry for being unclear, I was talking about usages of rootPackage in BuildStepImpl:

build/build/lib/src/builder/build_step_impl.dart

Lines 109 to 114 in e7b3d4e

    
           if (_reader is MultiPackageAssetReader) { 
        
             return (_reader as MultiPackageAssetReader) 
        
                 .findAssets(glob, package: inputId?.package ?? _rootPackage); 
        
           } else { 
        
             return _reader.findAssets(glob); 
        
           }

However, inputId is guaranteed to be non-null: We never set the rootPackage argument and the default is inputId.package from here. Since it was never used, I'm going to remove the field and parameter entirely.

jakemac53 · 2020-10-08T19:14:20Z

However, inputId is guaranteed to be non-null: We never set the rootPackage argument and the default is inputId.package from here. Since it was never used, I'm going to remove the field and parameter entirely.

Ok, I understand now :). I agree with your assessment as well, and I don't see any direct instantiations of BuildStepImpl outside of this package either so it should be safe to remove.

We should similarly deprecate the argument on runBuilder since all it does is pass it through to this constructor.

jakemac53

This is starting to look pretty close, I want to do another pass before we merge, which I will try to do today. I am on vacation next week though - so either I would need @natebosch to finalize the landing/publishing of this or I can pick it back up on/after 10/19 when I am back.

Speaking of, @natebosch did you want to do a review here as well?

build/lib/src/asset/exceptions.dart

jakemac53 · 2020-10-09T16:18:25Z

build_runner_core/lib/src/generate/options.dart

+///
+/// This is also the default list of files for targets in non-root packages when
+/// an explicit include is not provided.
+const List<String> defaultNonRootVisibleAssets = [


@natebosch particularly it might be good to get another set of eyes here - this list seems pretty reasonable to me though.

Possibly we should add 'CHANGELOG*'?

This list looks fine to me. I'd be in favor of adding CHANGELOG*

jakemac53 · 2020-10-27T14:34:54Z

Hey, sorry I just saw this one is still pending. Are you waiting on anything from us? Just a final review or did you have any more work pending here?

simolus3 · 2020-10-27T16:46:04Z

I just fixed the conflicts, so now I don't have any pending work here :)

natebosch · 2020-10-28T00:34:15Z

Oh interesting, I don't think this behavior was intentional - cc @natebosch can you think of why that would have been intentional?

I can't think of any reason we'd want canRead to throw...

natebosch

Overall this looks good to me. I'm also running this through some internal tests to see if there is anything breaking that we didn't intend.

build/CHANGELOG.md

natebosch

Internal tests look good.

jakemac53 · 2020-10-28T14:53:37Z

It looks like one failing test here and then we can merge/publish (possibly update the changelog as Nate suggests as well)

simolus3 added 7 commits September 17, 2020 14:24

Make public files configurable

7cdee73

Start writing some tests

a0e36e4

Merge branch 'master' into configure-public-files

aa23962

Only allow adding public assets, fix test

5225e61

Don't create synthetic nodes for invalid inputs

43a47a4

Clean up old tests, update docs

e847b86

Fix typos

720cca4

googlebot added the cla: yes Google is happy with the PR contributors label Oct 2, 2020

jakemac53 reviewed Oct 3, 2020

View reviewed changes

build/lib/src/asset/exceptions.dart Outdated Show resolved Hide resolved

simolus3 added 2 commits October 3, 2020 12:52

Bump versions, more tests for the target graph

4ec7e16

Add test for InvalidInputException.toString()

ee88a2a

simolus3 commented Oct 3, 2020

View reviewed changes

_test_common/lib/test_phases.dart Outdated Show resolved Hide resolved

docs/build_yaml_format.md Show resolved Hide resolved

simolus3 added 2 commits October 3, 2020 21:05

Add license and readme to default root sources

1a6ccd6

Don't invoke builders on invalid inputs

9f47cea

jakemac53 reviewed Oct 5, 2020

View reviewed changes

simolus3 added 2 commits October 6, 2020 09:33

Address review comments

dc4a476

Add missing dependency override to build_config

9ff178e

Deprecate rootPackage argument

0e21518

simolus3 added 2 commits October 9, 2020 12:27

Add old asset filter to finalized outputs

0eb6b9e

Fix pubspec, consequently remove rootPackage

b792c1d

jakemac53 reviewed Oct 9, 2020

View reviewed changes

jakemac53 approved these changes Oct 9, 2020

View reviewed changes

simolus3 added 2 commits October 9, 2020 20:10

positional -> named, include changelog

3ebbf6e

Don't throw for invalidInputs passed to canRead

6085062

simolus3 added 2 commits October 27, 2020 17:42

Merge branch 'master' into configure-public-files

9c2c594

Lift if from catch block

5d3d968

Format markdown file

d117c0f

natebosch reviewed Oct 28, 2020

View reviewed changes

build/CHANGELOG.md Outdated Show resolved Hide resolved

natebosch approved these changes Oct 28, 2020

View reviewed changes

Rephrase build changelog, fix failing test

63deab8

jakemac53 merged commit 064cd85 into dart-lang:master Oct 29, 2020

frencojobs mentioned this pull request Oct 30, 2020

Invalidate the build based on external dependency changes #967

Open

samyakkkk mentioned this pull request Jun 5, 2023

Custom build script usage causes Invalid Argument error. #3528

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand readable assets for non-root packages, make them configurable #2851

Expand readable assets for non-root packages, make them configurable #2851

simolus3 commented Oct 2, 2020 •

edited

Loading

jakemac53 left a comment

simolus3 left a comment •

edited

Loading

jakemac53 left a comment

jakemac53 Oct 5, 2020

simolus3 Oct 6, 2020

jakemac53 Oct 6, 2020

simolus3 Oct 6, 2020

natebosch commented Oct 5, 2020

simolus3 commented Oct 6, 2020

jakemac53 commented Oct 6, 2020

jakemac53 commented Oct 6, 2020

simolus3 commented Oct 6, 2020

jakemac53 commented Oct 8, 2020

simolus3 commented Oct 8, 2020

jakemac53 commented Oct 8, 2020

jakemac53 left a comment •

edited

Loading

jakemac53 Oct 9, 2020

natebosch Oct 28, 2020

jakemac53 commented Oct 27, 2020

simolus3 commented Oct 27, 2020

natebosch commented Oct 28, 2020

natebosch left a comment

natebosch left a comment

jakemac53 commented Oct 28, 2020

Expand readable assets for non-root packages, make them configurable #2851

Expand readable assets for non-root packages, make them configurable #2851

Conversation

simolus3 commented Oct 2, 2020 • edited Loading

jakemac53 left a comment

Choose a reason for hiding this comment

simolus3 left a comment • edited Loading

Choose a reason for hiding this comment

jakemac53 left a comment

Choose a reason for hiding this comment

jakemac53 Oct 5, 2020

Choose a reason for hiding this comment

simolus3 Oct 6, 2020

Choose a reason for hiding this comment

jakemac53 Oct 6, 2020

Choose a reason for hiding this comment

simolus3 Oct 6, 2020

Choose a reason for hiding this comment

natebosch commented Oct 5, 2020

simolus3 commented Oct 6, 2020

jakemac53 commented Oct 6, 2020

jakemac53 commented Oct 6, 2020

simolus3 commented Oct 6, 2020

jakemac53 commented Oct 8, 2020

simolus3 commented Oct 8, 2020

jakemac53 commented Oct 8, 2020

jakemac53 left a comment • edited Loading

Choose a reason for hiding this comment

jakemac53 Oct 9, 2020

Choose a reason for hiding this comment

natebosch Oct 28, 2020

Choose a reason for hiding this comment

jakemac53 commented Oct 27, 2020

simolus3 commented Oct 27, 2020

natebosch commented Oct 28, 2020

natebosch left a comment

Choose a reason for hiding this comment

natebosch left a comment

Choose a reason for hiding this comment

jakemac53 commented Oct 28, 2020

simolus3 commented Oct 2, 2020 •

edited

Loading

simolus3 left a comment •

edited

Loading

jakemac53 left a comment •

edited

Loading