Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flutter SDK dependency support for <flutter>/bin/cache/pkg #1775

Closed
mit-mit opened this issue Jan 8, 2018 · 18 comments

Comments

@mit-mit
Copy link
Member

commented Jan 8, 2018

Pub currently supports resolving packages located in <flutter>/packages when using an SDK dependency:

flutter:
    description: flutter
    source: sdk
    version: "0.0.41-dev"

This bug requests that it additionally also looks for matching packages in <flutter>/bin/cache/pkg when using a sdk_cache dependency:

sky_engine:
    description: flutter
    source: sdk_cache
@nex3

This comment has been minimized.

Copy link
Member

commented Jan 9, 2018

Do the packages in bin/cache/pkg ever overlap with the packages in packages? If not, can we just update the logic for the Flutter SDK to check in both place? Adding an entirely new source that only makes sense for Flutter is something I'd like to avoid.

@nex3 nex3 self-assigned this Jan 9, 2018

@mit-mit

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

I don't think so; @Hixie ?

@Hixie

This comment has been minimized.

Copy link

commented Jan 9, 2018

The version on sky_engine has never been updated as far as I'm aware. IMHO we should just drop it, not add any more dependencies on it. FWIW, we're about to drop all the version numbers in the flutter packages in the framework repo too. (It doesn't make sense to have these version numbers since the version will eventually change with every commit.) (see go/flutter-release)

Also currently the SDK dependency check is broken because we're publishing the version with the wrong file name, but we're planning on fixing that in the coming days.

@nex3

This comment has been minimized.

Copy link
Member

commented Jan 9, 2018

The version on sky_engine has never been updated as far as I'm aware. IMHO we should just drop it, not add any more dependencies on it.

Does this mean we should close out this issue?

@Hixie

This comment has been minimized.

Copy link

commented Jan 9, 2018

I don't know what led to this issue, so there may be underlying issues that need resolving? But yeah, as described I would recommend probably not supporting this.

@nex3

This comment has been minimized.

Copy link
Member

commented Jan 9, 2018

I believe the only motivating case was sky_engine, so I'll close this.

@nex3 nex3 closed this Jan 9, 2018

@mit-mit

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

@Hixie, flutter/flutter#13559 was the motivation for this. So we no longer depend on sky_engine?

@Hixie

This comment has been minimized.

Copy link

commented Jan 9, 2018

I assume we still depend on sky_engine (that's basically dart:ui), but there's only one sky_engine per checkout of the flutter repo (in bin/cache), so if we have established which Flutter SDK we're using, we know which engine to use as well, it shouldn't be ambiguous.

@mit-mit

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

Sure, but this bug wasn't meant to help with resolving the version, it was meant to help resolve the location of the package.

@Hixie

This comment has been minimized.

Copy link

commented Jan 10, 2018

The source: sdk_cache part seems fine to me. (If a little magical.)

@mit-mit

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

Re-opening to track there is still work to handle the location. Removed versioning from the top-most comment.

@mit-mit mit-mit reopened this Jan 10, 2018

@mit-mit

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

@Hixie, Natalie suggested above that rather than supporting a new sdk_cache source, she would just change the current sdk source to look first in <flutter>/packages, and then next look in <flutter>/bin/cache/pkg. Would that work?

@Hixie

This comment has been minimized.

Copy link

commented Jan 10, 2018

Seems fine to me.

I think if we wanted to make it a bit less magical then maybe we would have it only look in <flutter>, and then you'd provide a separate path to say where to look in <flutter>. But that's only worth it if the whole sdk mechanism is actually reusable by other projects at all. If sdk: flutter is a magical thing that turns on a special flutter mode then I'm fine with it magically knowing the paths and so forth.

Let's make sure we document this thoroughly either way. :-)

@nex3

This comment has been minimized.

Copy link
Member

commented Jan 10, 2018

If sdk: flutter is a magical thing that turns on a special flutter mode then I'm fine with it magically knowing the paths and so forth.

It is, more or less. The semantics of the SDK source are essentially "find these packages in an SDK-specific way", where that way may differ from SDK to SDK.

Let's make sure we document this thoroughly either way. :-)

The current docs just talk about the Flutter SDK "containing packages" without being explicit about where in the SDK those packages live. Is that enough?

@Hixie

This comment has been minimized.

Copy link

commented Jan 10, 2018

It doesn't necessarily have to be on the dartland.org Web site, but I think we should have some docs somewhere (maybe in the flutter pubspec.yaml where this mostly comes to a head) that talks about how pub is picking where to look, and so on.

@Hixie

This comment has been minimized.

Copy link

commented Jan 10, 2018

Specifically, imagine you and I and everyone else here move on to other things, and three years from now someone has to debug why this is not working any more. They're going to need an explanation of what on earth all this sdk: flutter magic actually means and where to start looking for fixes. (Maybe they moved the engine to a different cache directory or something, and want to know where to go to fix that lookup code.)

@nex3

This comment has been minimized.

Copy link
Member

commented Jan 10, 2018

I'll be sure to document the behavior in the pub source. As you say, it's probably also a good idea to document it on the Flutter side somewhere.

nex3 added a commit that referenced this issue Jan 10, 2018
nex3 added a commit that referenced this issue Jan 10, 2018
@Hixie

This comment has been minimized.

Copy link

commented Jan 10, 2018

Sounds good. Maybe on the Flutter side just have a link to the relevant source on the pub side, no need to document it twice (since then one is bound to get out of sync).

@nex3 nex3 closed this in #1778 Jan 10, 2018

nex3 added a commit that referenced this issue Jan 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.