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

.packages: Provide an Isolate.packageSpecUri getter #24524

Closed
kevmoo opened this issue Oct 7, 2015 · 14 comments
Closed

.packages: Provide an Isolate.packageSpecUri getter #24524

kevmoo opened this issue Oct 7, 2015 · 14 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-isolate
Milestone

Comments

@kevmoo
Copy link
Member

kevmoo commented Oct 7, 2015

Currently, it's very awkward to spawn a Dart process and reliably pass it the same package context that the parent context was using. You can pass the --package-root flag if Isolate.packageRoot is set, but Isolate.packageSpec is a map rather than a URI and as such can't be passed to a process. If we had Isolate.packageSpecUri, it could be passed as the --packages flag.

Having to create a packages data URI out of the map is clunky.

@kevmoo kevmoo added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-isolate labels Oct 7, 2015
@lrhn
Copy link
Member

lrhn commented Oct 7, 2015

It is the plan to have packageRoot and packageFile on Platform which returns the actual command line parameters. There is no guarantee that those parameters correspond to the current isolate's package resolution, but it will tell you how the Dart process was spawned.

Also, I think it's very bad style to start a new Dart process based on the current Dart process' settings. It's mixing tings that really belong in different settings (configuration of the current VM and data being processed by the running program).
Preferably, you should not need to execute a new Dart process - an Isolate.spawnUri will often be enough. If it isn't, you may be using special VM parameters that we can't guarantee the stability of.

@sethladd
Copy link
Contributor

sethladd commented Oct 7, 2015

Also, I think it's very bad style to start a new Dart process based on the current Dart process' settings

That may be true, but it would be useful to hear more about the use cases for this pattern.

you may be using special VM parameters that we can't guarantee the stability of.

For example? If I understand, --package-root and --packages are stable and can be relied on.

@nex3
Copy link
Member

nex3 commented Oct 7, 2015

It is the plan to have packageRoot and packageFile on Platform which returns the actual command line parameters. There is no guarantee that those parameters correspond to the current isolate's package resolution, but it will tell you how the Dart process was spawned.

This isn't sufficient. We need a reliable way to get a URI that communicates the current Isolate's package resolution. Otherwise, the logic will fail in tools like pub that start isolates for sub-applications.

Also, I think it's very bad style to start a new Dart process based on the current Dart process' settings. It's mixing tings that really belong in different settings (configuration of the current VM and data being processed by the running program).

I don't understand why it would be bad style. How else would you start a Dart subprocess in a test that can import libraries from the current package?

In practice, Dart code is run in many different entrypoint contexts—directly from the command line, via an isolate with various sorts of configuration, via a programmatically-executed process, etc. Our code should be written to work regardless of this context, and I don't see any way of doing that without copying over the package resolution configuration.

@lrhn
Copy link
Member

lrhn commented Oct 8, 2015

@sethladd

For example? If I understand, --package-root and --packages are stable and can be relied on.

Yes, --package-root, --packages, --checked and -Dfoo=bar are all stable and can be relied on. There are parameters corresponding to those on spawnUri (not all implemented yet, but that's the goal, please help pushing for them to be implemented).

The parameters I'm talking about are VM specific things like --old_gen_heap_size, which you may want to change in extreme cases, but it's not something I'd want you to rely on in general.

@nex3

This isn't sufficient. We need a reliable way to get a URI that communicates the current Isolate's package resolution. Otherwise, the logic will fail in tools like pub that start isolates for sub-applications.

That assumes that there is a URI. If the current isolate was started using Isolate.spawnUri with a packages map, there won't be any URI at all. The mapping is available as Isolate.packageMap.

We bootstrap this by having the initial mapping stored in a file, but after that, mappings are represented by Map objects, not URIs to files. The file is just an external representation of the actual mapping.

If you really need to start a new Dart process with a custom mapping, you need to write that mapping to a file, or reuse an existing file when you know that one exists. Or use a data: URI if line length permits (and it's implemented, I don't know if it is).

I don't understand why it would be bad style. How else would you start a Dart subprocess in a test that can import libraries from the current package?

It should be sufficient to use Isolate.spawnUri unless you want to start the process with extra VM-specific parameters. We haven't implemented all the spawnUri parameters yet, but that is the goal we aim towards. We shouldn't add features to work around other missing features, we should just implement the missing features.

@lrhn lrhn added the closed-not-planned Closed as we don't intend to take action on the reported issue label Oct 8, 2015
@lrhn
Copy link
Member

lrhn commented Oct 8, 2015

The internal representation of a package-resolution map is a Map. There is not guaranteed to be any URI corresponding to it, and even if there is, that's only pointing to an external representation of the actual map.
We do not plan to expose the URI used to load that map other than as the actual command line parameter specified (in dart:io Platform).

@lrhn lrhn closed this as completed Oct 8, 2015
@nex3
Copy link
Member

nex3 commented Oct 8, 2015

The VM's internal representation isn't relevant—the Dart team writes the VM and can change it as necessary. We need to focus on user needs. If the VM doesn't currently keep around the original URI, then it should start doing so in order to support this feature. If we insist on supporting a Dart map parameter to Isolate.spawnUri, the VM/core libraries should convert that to a data: URI because that's what's useful to the user.

It should be sufficient to use Isolate.spawnUri unless you want to start the process with extra VM-specific parameters. We haven't implemented all the spawnUri parameters yet, but that is the goal we aim towards. We shouldn't add features to work around other missing features, we should just implement the missing features.

Regardless of what "should be", right now it's very clearly not sufficient. There are tons of things that can be done with a process that can't with an Isolate: a process can have different environment variables, can be detached, can have its stdio interacted with, can be signaled, can safely call exit(), can have its exit code read. When you're doing something like testing a command-line API, Isolates are nowhere near sufficient.

Maybe all of these features could somehow be added to the Isolate API, but the fact is they're not there now. If we want to move towards a world without package symlinks in the next few releases, we need to have a package-spec-based alternative for everything people were using them for, this included.

@lrhn
Copy link
Member

lrhn commented Oct 9, 2015

This is not about the VM's internal representation, it's about the Dart libraries choice of representation.
I have no idea what the VM does internally, but it provides the data as a Map because that's what we want it to do. A package resolution is specified by a mapping from package name to URI. In Dart code, the representation of that is a Map from String to Uri. The .packages file is just a serialization of that logical mapping, it is not the mapping itself. In Dart code, maps are the objects you should use.

Regardless of what "should be", right now it's very clearly not sufficient.

I know that. The solution to that is to implement the missing features, not to implement other features to work around the missing features. I'm all for prioritizing this implementation! If you can use existing features to work around the ones that are missing, great, but adding features that won't be necessary in the long run is a waste of time.

If you really need to test a command line application, and need to start a new Dart process, you should control its parameters explicitly, not rely on reflecting what the current running Dart process happens to be using. That sound like it would be very fragile.
If you want to do it anyway, you should be able to deduce how the current process was started from the Platform properties: script, packageRoot and packagesFile (or whatever the last one is named, it's the command line --packages parameter). You can start a new process with the same parameters with very little effort. There is no need for the VM to produce a resolved version of the command line parameter.

If anything, we can make a helper package for spawning DartVM processes which can do the stuff for you. That would be the right place for such logic - take one DartVM process (the current) and create another one from that with some changes applied.

@sethladd
Copy link
Contributor

sethladd commented Oct 9, 2015

packagesFile

I don't see that in Platform: https://api.dartlang.org/134093/dart-io/Platform-class.html Are we intending to add that? If so, that might be what @nex3 would like.

I guess we could use https://api.dartlang.org/134093/dart-io/Platform/executableArguments.html ?

@lrhn
Copy link
Member

lrhn commented Oct 11, 2015

Yes, that is the intention, to match packageRoot.
(Still not sure whether it should be called packages, packagesFile or packageFile ... or something else, it will contain the content of the packages command line parameter).

@nex3
Copy link
Member

nex3 commented Oct 13, 2015

I can't imagine we'll ever have isolates that match the full functionality of subprocesses. If we somehow do, it will be years from now. And even then, it's not clear to me that it will be concretely better in any way to test subprocesses via isolates than it will via Process. Process is always going to be closer to real world invocations, and that's important for tests.

We need this feature, and we need it soon, or our users—including us—won't be able to make their code work without symlinks. There's no real cost to exposing this other than a small amount of implementation time, and we have a clear concrete use-case for it.

Platform.packagesFile won't be sufficient, assuming it works similarly to Platform.packageRoot. It will be executable-global, which means it can't be reliably used to pass the current isolate's package resolution information to a subprocess. Also, it only matches the exact command-line argument, it won't be available if it was discovered implicitly and it may be invalid if the process's working directory has ever changed.

@lrhn
Copy link
Member

lrhn commented Oct 14, 2015

If you need to use process, by all means use process. But it's a completely different process then, and I don't think it should rely implicitly on however the current process was launched, and much less on how a specific isolate was spawned. If you really want to reify the current isolate's package resolution, we have a helper package for that (use Isolate.packageMap to get the map, and package:pacakge_config to write it to a file, then use that file, or use a data: URI if we support that and it fits on the command line).

If your isolate's package resolution doesn't match the original command line parameter, then there is no package-spec file. If there is a package root, you can get that from Isolate.packageRoot, otherwise Isolate.packageMap is the all you have - that map came from a map originally provided to spawnUri, there is no file we can point to. So, either Platform.packageFile is useful, or there is no useful file URI to provide.

@nex3
Copy link
Member

nex3 commented Oct 14, 2015

If you really want to reify the current isolate's package resolution

This isn't something we can just write off as "I guess you can use this hacky workaround". It's an important use-case that your customers—including me—rely on. Having to depend on a new package and write a file to disk just to spawn a process is onerous and unperformant, especially since in the vast majority of cases the package spec we want to use is already there.

If your isolate's package resolution doesn't match the original command line parameter, then there is no package-spec file.

If the package spec was located based on the entrypoint's location rather than being passed in explicitly, there won't be a command-line parameter but there will be a file (or an HTTP URL).

If there is a package root, you can get that from Isolate.packageRoot, otherwise Isolate.packageMap is the all you have - that map came from a map originally provided to spawnUri, there is no file we can point to. So, either Platform.packageFile is useful, or there is no useful file URI to provide.

As I've pointed out repeatedly, this is a serious flaw in the API we're adding to spawnUri. If spawnUri took a URI rather than a map, that URI could be exposed as Isolate.packageSpecUri. As it is, in the common case where the package map comes from a file, that information is discarded.

@kevmoo kevmoo removed the closed-not-planned Closed as we don't intend to take action on the reported issue label Oct 16, 2015
@kevmoo kevmoo added this to the 1.13 milestone Oct 16, 2015
@kevmoo
Copy link
Member Author

kevmoo commented Oct 16, 2015

Re-opening per discussion in #23951

@kevmoo kevmoo reopened this Oct 16, 2015
@mit-mit mit-mit changed the title Provide an Isolate.packageSpecUri getter .packages: Provide an Isolate.packageSpecUri getter Oct 20, 2015
@kevmoo kevmoo modified the milestones: 1.14, 1.13 Oct 22, 2015
@mit-mit
Copy link
Member

mit-mit commented Jan 14, 2016

This was landed

@mit-mit mit-mit closed this as completed Jan 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-isolate
Projects
None yet
Development

No branches or pull requests

5 participants