search

disruptek / nimph

Nim package hierarchy manager from the future 🧚
MIT License
159 stars 10 forks source link

nim.cfg --path removal may be buggy #112

Closed ghost closed 4 years ago

ghost commented 4 years ago

Consider this scenario: User updates dependency foo, this update (re)moves foo/submodule. User runs tests, they pass User pushes to master, build fails for CIs or other users: Error: cannot open file: foo/submodule. The old path was used.

I would enjoy giving nimph some authority on cleaning up the nim.cfg, even if opt in.

--out_dir="bin"
--clear_nimble_path
--nimble_path="$config/deps/pkgs/"
--path="$config/deps/pkgs/std_ext-1.5.2/src/"
--path="$config/deps/pkgs/clang_tooling-0.1.0/src/" # I propose this be removed
--path="$config/deps/pkgs/clang_tooling-0.1.1/src/"
disruptek commented 4 years ago

You're right about what we want. I'm not sure you're right about what we have. :smile:

If you nimph upgrade clang_tooling, then it won't be duplicated -- it will just be rolled from 0.1.0 to 0.1.1 and left in the same directory. When a directory doesn't exist, nimph doctor removes it from explicit path statements in nim.cfg.

If clang_tooling is not upgraded by nimph, then we should be warning of the name shadow. nimph doctor should clear the path from the nim.cfg if it won't be used due to shadowing. I dunno if this is implemented yet or if there's just a warning to remind me to do so.

If you clone a package into the environment and then you don't add it to requirements, nimph will leave it in nim.cfg for you so that it still works while you evaluate it. It's annoying to have other doctor operations remove test or development requirements.

Also, by design, the behavior may be slightly different (less invasive) for packages that reside outside $projectpath or $config.

Everything can be improved. Tell me what it should do. :wink:

disruptek commented 4 years ago

Also, nimph doesn't currently rename package directories that it upgrades. This is a 1-line fix that I guess just hasn't annoyed me enough. But, it could be confusing both to nimph and nimble users.

edit: actually, nimph does rename upgrades/downgrades; it just doesn't rename directories to correct inaccurate names bugfix?

ghost commented 4 years ago

I think I did clone clang_tooling initially. Added a requirement for it. Updated the requirement. Then observed the dual paths and pkgs.

I failed to reproduce the exact bug without tagging a new release, but will try to reproduce again when I do.

A possibly related minor bug I encountered when trying to reproduce: foo.nimble: requires "https://github.com/dumjyl/clang-tooling-nim 0.1.0" nimph doctor with a fresh nim.cfg update foo.nimble: requires "https://github.com/dumjyl/clang-tooling-nim >= 0.1.1" nimph doctor leaves me with duplicated paths in nim.cfg:

--path="$config/deps/pkgs/clang_tooling-0.1.1/src/"
--path="$config/deps/pkgs/clang_tooling-0.1.1/src/"
disruptek commented 4 years ago

Oh, I see what happens. The doctor runs and looks for packages to satisfy the current requirements. She doesn't find clang-tooling-nim as a match because it is too low a version. So she installs a new clone at the version she wants. Since the shadowed version isn't detected as such (yet), nimph doesn't have a good reason to remove it from the nim.cfg.

I'll fix it to look for packages that could satisfy the requirements, but if the package isn't up-to-date with remotes, we won't know. I haven't written git fetch yet in gittyup, and I'd want that for safety.