Properly fix issue #2691 by reverting to the original scanning behavior #2865
Conversation
|
✅ PR OK, no changes in deprecations or warnings Total deprecations: 0 Total warnings: 0 Build statistics: statistics (-before, +after)
executable size=5368480 bin/dub
rough build time=62sFull build output |
|
Maybe this should go into stable? |
aad4a0e
to
911d4c3
Compare
Reverts to the scanning/warning behavior that was in place before the warnings for the new package directory structure were implemented.
911d4c3
to
6f8d633
Compare
|
@s-ludwig : If we go with the
Why would one use Also Another approach we could take is that packages in |
Because add-path doesn't require you to make any changes to the packages, no matter if you are working with cached packages, checked out versions, or where exactly you want to check them out.
No, that's wrong. It will query the git branch and latest tag to determine a version. Since add-path directories have precedence over cached packages, this allows you to work seamlessly on a version branch, add new commits, while dependent packages still pick it up when building.
I'm using the existing approach (as well as anyone I'm working together with) since a long time and it's absolutely vital to my workflow. It simply works as expected. By the way, if someone doesn't use add-path, there is no reason why that would have to impact package lookup performance, right? As far as I see it, this feature is really a completely isolated use case that got conflated with the cache directory purely coincidentally, based on the fact that it got implemented with the same logic. In perspective, the code for handling cached packages should simply be moved out to a different class/struct as soon as the forward probing for the new structure gets implemented. |
Only because This doesn't mean the feature is not valuable, obviously you're obviously a prime user of it, but it has its limitations. Another one that comes to mind immediately is that you would have to ensure, as a user, that your package is checked out under the right directory name if you're working on packages that leak outside their directory (arsd, ae). It would work, but that's another potential issue you have to be aware of.
Correct, I was actually looking at that this week. |
This is literally the way this is meant to be used. Requiring the use of SCM checkouts could be a change that might make sense, since I can't imaging that anyone uses it with plain copies of packages (since, as you mentioned, that doesn't work anyway due to the missing version field). But please recognize that independent of the functional arguments, we are talking about a change that was introduced on the stable branch, on a stable version track, modifying test cases, without proper discussion and without proper review. IMHO, it doesn't fit a project at this stage and exposure for things to be handled this way.
That is a problem with the package setup and has nothing to do with this feature, you'll have to make sure to get that right no matter how you make it visible to DUB. |
It makes sense with your explanation, this was not obvious to me before.
If you look at the tests - this is exactly what they do! I think that was an abuse of the feature used at the time the tests were written, but that is part of why I got the impression that this was only about adding "yet another path".
I want to highlight that the change didn't remove any functionality. Using the age-tested structure still works but serves you a deprecation message / warning. All it did was make it work with the new structure. The test would pass before or after the change, I just wanted to reduce the number of deprecations the test suite was throwing.
Even so, it's a footgun. The more baked-in assumption we make in how a feature is used, the most likely it is to be misused. Note that my messages were not intended to challenge the need for a change in the direction in that PR. I just wanted to build a better understanding so I know what are the requirements around this feature in the future. I will try to review this ASAP. |
|
Fair enough, I do recognize that this feature is not well communicated as it stands and I should probably have done something about that a long time ago.
My biggest problem is a) that changing the tests like that manifests the wrong approach, making it difficult to recognize and correct later, and b) that now a harmless bug (spurious warning) is turned into either a regression (when it gets reverted later) or introduces a use(case)less (and error prone*) feature that needs to stay supported for a long time. But mainly all I'm saying is that the way this got introduced into the code base is rather unfortunate, even though I'm generally all for reducing unneeded friction in the development process.
As far as I see it, the fact that an import path is set outside of the repository/package directory and the reliance on a particular name for folder where the code resides is the real issue/footgun here. I can understand how this is both a historical issue and how it is a nice setup for the respective authors of those packages. However, for a package that is originally intended for public consumption (which was not the case here, AFAIK), where it is uncommon to integrate packages using By the way, if my wording came across as harsh at some point, that was not my intention. This particular feature just happens to repeatedly fall victim to collateral damage, which admittedly is quite annoying and should ideally be prohibited by proper test cases. I will put writing better documentation and test cases on my TODO list, to at least have done my part. * On my setup, I had a bunch of errors being printed, because some repositories have sub folders that match the NAME/VERSION/NAME pattern and it tries to load those sub folders as packages. So this part also actually is an additional regression over the previous behavior. |
source/dub/packagemanager.d
Outdated
| if (!mgr.existsDirectory(vers_path)) continue; | ||
| packageFile = Package.findPackageFile(vers_path); | ||
| loadInternal(vers_path, packageFile); | ||
| if (mgr.isManagedPath(path)) { |
There was a problem hiding this comment.
We could just use this.isManaged here couldn't we ?
There was a problem hiding this comment.
It think so, yes, I'll try. This is just the expression that was there before.
| } else { | ||
| // Unmanaged directories (dub add-path) are always stored as a | ||
| // flat list of packages, as these are the working copies managed | ||
| // by the user. The nested structure should not be supported, | ||
| // even optionally, because that would lead to bogus "no package | ||
| // file found" errors in case the internal directory structure | ||
| // accidentally matches the $NAME/$VERSION/$NAME scheme | ||
| if (!packageFile.empty) | ||
| loadInternal(pack_path, packageFile); | ||
| } |
There was a problem hiding this comment.
Ideally we would just split the logic to scan searchPaths from the logic that scans packagePath.
There was a problem hiding this comment.
I've looked at the code and it seems like this will be a bigger refactoring that warrants its own PR. I don't have a fully clear picture, yet, but is this what we have now?:
- Once per "package", "user", "system"
- One managed cache path
- One set of package overrides
- One set of add-path paths
- One set of add-local package locations
- Custom managed cache paths (read-only)
- DUBPATH search path (managed/flat?)
There was a problem hiding this comment.
That's sounds about right. Although overrides are deprecated. And the PackageManager may be in "bare" mode. And one may override where "user" and "system" are.
|
anything to improve on the documentation from this PR? |
|
e.g. in |
|
@WebFreak001 https://dub.pm/dub-reference/dependencies/#dub-add-path looks like a good place for the detailed description, and I'd probably put a link to that page in the CLI documentation of |
Reverts to the scanning/warning behavior that was in place before the warnings for the new package directory structure were implemented.
This reverts #2780.