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

Add the ability to specify an optional local path as well as a locator #274

Open
chalcolith opened this issue Jan 7, 2025 · 7 comments
Open
Assignees
Labels
discuss during sync Should be discussed during an upcoming sync enhancement New feature or request

Comments

@chalcolith
Copy link
Member

chalcolith commented Jan 7, 2025

Sometimes I want to develop against a local copy of a dependency rather than a version e.g. at GitHub, but when I build in CI I want to use the GitHub version. (Like Cargo allows: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#multiple-locations)

I'd like to change the dependency JSON to allow something like:

"deps": [
  {
    "path": "../my_local_checkout",
    "locator": "github.com/myaccount/myrepo",
    "version": "1.1.1"
  }
]

where path is optional.

During corral fetch, it will check the local path first, and if it exists, will do nothing. If it does not exist, it will clone/fetch the GitHub repo. During corral run, it will set PONYPATH to the local path if it exists, otherwise it will use the GitHub repo.

@chalcolith chalcolith added the enhancement New feature or request label Jan 7, 2025
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jan 7, 2025
@chalcolith chalcolith self-assigned this Jan 7, 2025
@SeanTAllen
Copy link
Member

I'm not in favor of this. Let's discuss before you put effort into this.

@SeanTAllen
Copy link
Member

Gordon and I discussed. We are definitely not in agreement here. We should definitely have some discussion with a larger group. I have a number of concerns that come from this solves a minor bit of pain in terms of developer convenience but introduces a path for "ambient dependencies" where I can't look at a version of corral and corral.json and know what my dependencies are supposed to be. There are a lot of problems that I think arise from dependency resolution that is dependent upon the environment that it will be run in.

I would like to eliminate all forms of "ambient dependencies" that could happen in checked in code from Pony. As some folks know, I have a proposal I need to actual get out as an RFC for changes to dependency management that would in part eliminate the PONYPATH for similar reasons- ambient dependencies.

@chalcolith
Copy link
Member Author

I'm afraid I'm still not sure what Sean means by "ambient dependency". For me, the phrase implies a dependency that is undeclared or automatic.

However, this proposal is to allow an explicit, named, dependency that must correspond to a physical directory that is deliberately created offline by a user in order to be used. To me this does not meet the definition of "ambient". I can explicitly tell what the dependencies are: if I look at my corral.json and see a local path (which is already allowed in the "locator" property), I can navigate to that directory and examine it.

I can see several possible failure cases for this feature:

  • A developer has a directory of the same name that does not contain Pony code. In this case their build will fail and they will need to investigate.
  • A developer has an old copy or clone of a repo. In this case their build may succeed by accident, but is likely to fail.
  • A CI build machine has a directory of the same name containing non-Pony code. In this case the build will fail and need to be investigated.
  • An attacker may create a directory on a CI machine with malicious code. However, if an attacker has access to a CI machine, they may modify any code at will and this proposed change does not increase our vulnerability.

Sean, can you clarify, please:

  • What is your definition of "ambient dependency"?
  • What can a user not tell about their build from examining their "corral.json" and its associated local paths (whether in the "locator" fields or the proposed new "path" field)?

@SeanTAllen
Copy link
Member

You can't tell from looking at a corral.json what the dependency to be used is without looking at the local environment. This can be pushed and MAYBE override the location given. or MAYBE not. There is no way to no what the dependency will be without looking at the environment that it will be run in. That's an ambient dependency. The dependency is entirely dependent on the environment it is run in. If a path field is checked in then it might be something for that path, it might be the locator.

If a local bundle path is used in the locator on the other hand then it will be whatever is at said location where I wouldn't trust anything that goes outside of the existing libraries package hierarchy.

What can a user not tell about their build from examining their "corral.json" and its associated local paths (whether in the "locator" fields or the proposed new "path" field)?

That is exactly my problem. I must know about "associated local paths" like "/tmp/something_here" which may or may not have a thing. As part of the proposal I have (but not yet public) for changing dependency management, a package/module would not be able to reference things at a location like /tmp/something_here because that becomes an ambient dependency.

An attacker may create a directory on a CI machine with malicious code. However, if an attacker has access to a CI machine, they may modify any code at will and this proposed change does not increase our vulnerability.

This is not necessarily true. What an attacker has access to could be many different things including control over only some locations.


Note again, you can already do a local override of this sort by manipulating the PONYPATH which I also want to get rid of.

@SeanTAllen
Copy link
Member

If other people really want this, I won't fight to hard because I want to get rid of corral in general and everything about our existing dependency resolution so this would all go away if I get that accepted. That said, I would feel much more comfortable letting something like this in knowing that I could start work and people would accept my proposals for the large sweeping changes that I want to do.

I believe everyone on the core team is familiar with at least the general shape of those changes at this point.

@SeanTAllen
Copy link
Member

However, this proposal is to allow an explicit, named, dependency that must correspond to a physical directory that is deliberately created offline by a user in order to be used.

This already exists with the concept of a local bundle.

I disagree with your formulation of what this proposal is. I would say that it is to allow a second means to do what local bundle provides that will allow the override of locator IFF the path exists. As you have stated in our offline conversation so that you can check both in and use the local dependency locally and a different dependency remotely.

@chalcolith
Copy link
Member Author

I believe that my phrase "that must correspond to a physical directory ... in order to be used" is logically equivalent to your phrase "IFF the path exists". I don't think we disagree on what the proposal is, but on whether the convenience for development is worth possible downsides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss during sync Should be discussed during an upcoming sync enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants