search

erlef / rebar3_hex

Rebar3 Hex library
Apache License 2.0
101 stars 49 forks source link

depencies are set to hexpm by default #312

Closed jbdamiano closed 2 years ago

jbdamiano commented 2 years ago

depencies are set to hexpm by default to avoid to research it on a private repository

fix #311

tsloughter commented 2 years ago

What about when it is published to a specific organization? Or was that already handled and only default hexpm wasn't?

jbdamiano commented 2 years ago

It is handled in my case. The problem is only in public hexpm

starbelly commented 2 years ago

@tsloughter @jbdamiano It should just be the repo name, I don't see any examples in hexpm, nor hex_core, nor mix hex where repo:org is used, that makes sense to me since in hex the org is considered a distinct entity from the repo. Since I say "I believe" here, we probably want to double check.

There's one change that needs to be made regardless @jbdamiano, we don't want to hard code "hexpm" here, instead we'll have to provide the parent repo as an argument to get_deps/2 . Around here https://github.com/erlef/rebar3_hex/blob/110f55affe28713f0208cf73cdcc22351f81b704/src/rebar3_hex_build.erl#L324 you can call get_repo/1 (defined in the same module, rebar3_hex_build) to get the repo, we'll then either need to pass in the repo returned to get_deps/1 either whole or just the parent repo name. Passing just the parent makes sense to me I think, so you can match on the parent with #{parent := Parent} = get_repo(State). Then just include that in the requirement. We'll need to update the tests in test/rebar3_hex_app_SUITE.erl along with this change.

Does that make sense?

Edit:

There's a problem with both approaches. It assumes that one of the deps your app has came from the repo your publishing to, which may not be the case. It may be best for now just to default to <<"hexpm">> with a plan to make changes in rebar3 to accomidate more complex cases. Specifically, that information for this step is not currently available (i.e., repo, org, etc.) in deps iirc. @tsloughter thoughts?

Edit:

Confirmed that it should never be repo:org in the requirements proplist for the repository key.

tsloughter commented 2 years ago

Oh wait, this is for figuring out the dep information to be part of a package when published? It shouldn't assume the dep is from hexpm.

Is the original issue that when it doesn't set this information the fetching then assumes the dep package is from the same org as the parent?

Could this also be temporarily solved by having it not default to assuming it is from the org the parent is in?

starbelly commented 2 years ago

Oh wait, this is for figuring out the dep information to be part of a package when published? It shouldn't assume the dep is from hexpm.

Yeah, I agree. I suppose my thought on this was let's not let perfect be the enemy of good (i.e., currently the case this PR revolves around simply doesn't work and is a highly sought after featrue, I ran into it myself a few weeks ago).

Is the original issue that when it doesn't set this information the fetching then assumes the dep package is from the same org as the parent?

Correct.

Could this also be temporarily solved by having it not default to assuming it is from the org the parent is in?

Yeah, we could brute force work around it by searching all available repos, but there's a gotcha in there. Say I publish package foo 0.1.0 to hexpm and foo 0.1.0 to my own hex server, which one do we pick?

I think ultimately and want I wanted us to avoid was having to make changes in rebar3 in regards to deps and locks. I believe as discussed on slack at one point we need to support one to two things :

  1. Allow the user to specify what dep a repo should come from ala mix ,this includes specifying the org.

  2. Write the repo (not org part) into lock files, this requies a new lock file version, so we'd end up maintaining 3 lock file versions.

tsloughter commented 2 years ago

I do not want repo in the lock file or rebar.config as part of the dep.

I think we should probably not attempt to treat orgs as repos anymore. They are not repos and acting like they are has caused a lot of pain.

Putting the org in a config with a dep and adding it to the lock file for each dep would then be fine with me.

Should we go ahead and merge this?

starbelly commented 2 years ago

I do not want repo in the lock file or rebar.config as part of the dep.

I'm only suggesting this as that's what mix does and it makes sense to me, maybe for rebar it doesn't have to be that way. In other words, if I have a private dep published to myorg on hexpm. The lock file ends up containing [repo: "hexpm:myorg", ..." but let's discuss that further in an another issue.

I think we should probably not attempt to treat orgs as repos anymore. They are not repos and acting like they are has caused a lot of pain.

Agreed.

Putting the org in a config with a dep and adding it to the lock file for each dep would then be fine with me.

👍

Should we go ahead and merge this?

I think it should be fine. It probably shouldn't be hard coded to hexpm, I can fix that up in another PR. I'll need to test all this a good bit before we cut a release.