Exclude importPaths that start with ".." #2637
Conversation
|
✅ PR OK, no changes in deprecations or warnings Total deprecations: 15 Total warnings: 0 Build statistics: statistics (-before, +after)
-executable size=5276640 bin/dub
-rough build time=75s
+executable size=5272544 bin/dub
+rough build time=76sFull build output |
|
can you add a test for this? |
|
That's tricky, as the thing to test here is that Dub does not try to read this unrelated file / enter this unrelated directory. It does gracefully skip broken symlinks, so aside something like a logging / error-injecting FUSE filesystem I can't think of anything right now. |
|
If I do |
|
Added it as I can't think of anything better right now, and I see the tests are POSIX-only anyway. |
|
you still need to put in the .no_build, .no_test, .no_run files so the CI tests don't try to do those. |
source/dub/recipe/packagerecipe.d
Outdated
| // collect import files and remove sources | ||
| import std.algorithm : copy, setDifference; | ||
|
|
||
| auto importFiles = | ||
| chain(collectFiles(importPaths, "*.{d,di}"), collectFiles(cImportPaths, "*.h")) | ||
| chain(collectFiles(projectImportPath, "*.{d,di}"), collectFiles(cImportPaths, "*.h")) |
There was a problem hiding this comment.
don't forget to also do this for cImportPaths
There was a problem hiding this comment.
Um... why? Well OK, maybe let's just do it for all paths. I don't see why not. Let's see what breaks.
There was a problem hiding this comment.
oh no, you definitely want to be able to specify source files with .., since for sub-packages they might want to share some kind of info file across all sub-packages that for example just contain some compile time helpers.
Also not all globs in parent directory scan recursively, but only specify single files, where this kind of exclusion would seem really arbitrary to the user.
|
BTW I'm seeing zero documentation on how to run the test suite. A section in the README, or a |
CyberShadow
force-pushed
the
pull-20230508-103510
branch
from
b101765
to
b45838d
Compare
This mainly targets `importPaths ".."`, but to some degree applies to all paths that may occur in a package description file. One use case for requiring such a declaration is when the module root is outside the Dub project root. For example, the library "foo" containing a D module "foo.bar" may be in a directory called "foo" and have "bar.d" at the directory root (same directory as "dub.sdl"). Currently, setting importPaths to ".." has the unpleasant side effect of scanning the entire parent directory (which may host other, unrelated projects). Dub does this for reasons such as caching; the build process itself does not require a list of all files that may or may not be imported, as the compiler discovers them not by globbing, but by path construction and file existence checks, based on the names of imported modules and the list of import paths specified with -I. Ideally, Dub should NEVER recursively glob the importPaths list, and instead communicate with the compiler to discover the full list of files that were actually imported (e.g. from compilers' verbose output). However, this change (which should not affect canonical use cases) should facilitate this particular directory structure.
CyberShadow
force-pushed
the
pull-20230508-103510
branch
from
b45838d
to
33f3889
Compare
|
here are some projects which rely on .. in globbing, to test if they still work properly: for source paths: for source files: for import paths: it seems that this is even intended documented behavior, although I don't get what it has to do with git submodules. I think it would have made more sense that |
|
I think what we should do for import paths: (and C import paths, since they are the same thing for some compilers)
this way we also have proper semantics for importPaths ".." that users can utilize effectively and with solid guarantees. |
Hmm, I think the intent there is to put Dub-related files in their own directory? I'm not sure what to do here. Is it even possible to publish this to the Dub registry? But even if not,
Makes sense.
Ah, that one's mine! Looks like I made this a long time ago to illustrate the same problem.
Sounds like a plan... though, it does bother me that we don't have a solid and defensible mechanism which would be used to decide which sets of paths are filtered, and how. Perhaps this is the wrong approach entirely, and we need something like a |
I think moduleRoot could be one thing how this could be done, but I think keeping "name" for it already makes sense, although it breaks when users use Do you have a requirement / use-case why you need to control the parent folder/current folder relationship? |
Yeah sorry, I meant that Dub could use
Well the Dub package name doesn't always correspond to the module name, and special symbols aren't the only case. Probably the most prominent example I'm aware of is
It's Of course it would be nice to understand and fix the problem as generally as possible. For example, what about something like a package called |
What do you think?