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

Misleading error on update where dependency bundle is not found #99

Open
Theodus opened this issue May 6, 2020 · 5 comments
Open

Misleading error on update where dependency bundle is not found #99

Theodus opened this issue May 6, 2020 · 5 comments
Labels
bug Something isn't working help wanted Extra attention is needed needs discussion Needs to be discussed further

Comments

@Theodus
Copy link

Theodus commented May 6, 2020

corral.json:

{
  "deps": [
    {
      "locator": "github.com/ponylang/peg.git",
      "version": "0.1.0"
    }
  ],
  "info": {}
}
$ corral update
update: updating from /home/theodus/src/wip/changelog-tool
git cloning github.com/ponylang/peg.git into /home/theodus/src/wip/changelog-tool/_repos/github_com_ponylang_peg_git
update: will load dep: github.com/ponylang/peg @ 0.1.0
git checking out @0.1.0 into /home/theodus/src/wip/changelog-tool/_corral/github_com_ponylang_peg
Error loading dep bundle: github_com_ponylang_peg

The corresponding warning for the fetch command should also mark itself as a warning:

$ corral fetch
fetch: fetching from /home/theodus/src/wip/changelog-tool
git fetching github.com/ponylang/peg.git into /home/theodus/src/wip/changelog-tool/_repos/github_com_ponylang_peg_git
fetch: fetching dep: github.com/ponylang/peg @ 0.1.0
git checking out @0.1.0 into /home/theodus/src/wip/changelog-tool/_corral/github_com_ponylang_peg
No dep bundle for: github.com/ponylang/peg.git
@Theodus Theodus added the enhancement New feature or request label May 6, 2020
@SeanTAllen SeanTAllen added bug Something isn't working help wanted Extra attention is needed needs discussion Needs to be discussed further and removed enhancement New feature or request labels May 18, 2020
@SeanTAllen
Copy link
Member

The problem with this comes down to the relationship between a project and a bundle.

Corral internally allows for the concept of a project without a bundle, but the update code will always try to load and then consider it an error. The lack of bundle.json for transitive dependencies shouldn't be an error in my mind.

The specific error is here:

https://github.com/ponylang/corral/blob/master/corral/bundle/project.pony#L62

The big question here is...

should there be an error message when you use a project that doesn't have a corral.json?

I think maybe no and that we don't output anything. (Nothing because I think warnings are dumb, things should either be an error or not).

The argument in favor of making this an error is that it will help push everyone to start using corral.json in all their projects.

But if an error it should stop processing and let you know it will not continue. This would put pressure on the Pony community to bring libraries up to date with working with corral. It might mean that in order to use a dependency, that you have to fork it to add a bundle.json, but if a library is abandoned, that might not be the worst idea.

@adri326
Copy link
Member

adri326 commented Jun 23, 2020

If an error is kept, it should say that the specific dependency is missing a corral.json file:

Error loading dep bundle: github_com_ponylang_peg is missing a corral.json config file

@CandleCandle
Copy link

I am in favour of encouraging people to use the same patterns, however, I would not want to block a developer from using a 3rd party dependency if it is the correct tool for the job.

If a dependency does not have a corral.json then corral should behave as if it had no dependencies. The message can be something like:

Warning: loading dep bundle: github_com_ponylang_peg is missing a corral.json, no transitive dependencies will be added.

Informing the user that this is something that should be fixed and may have detrimental effects if it is not fixed, however, it should not be a terminal error.

An example:

MainProject
  |- CorralBasedA
  | \- CorralBasedC 
  |- CorralBasedB
  \- NonCorralBased

In the above example, NonCorralBased may have dependencies, but it is role of the developer of MainProject to add those dependencies to MainProject

@niclash
Copy link

niclash commented Jul 1, 2020

I am with @SeanTAllen that warnings are bad, especially when they refer to something that I can't fix. Having a dependency on another project that is lacking corral.json (and bundle.json) is such a case. Consume it as if no dependencies and give no warnings is IMHO the right way.

@CandleCandle
Copy link

Consume it as if no dependencies

This is the key point, feedback to the user is optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed needs discussion Needs to be discussed further
Projects
None yet
Development

No branches or pull requests

5 participants