dlang / dub

Package and build management system for D
MIT License
673 stars 230 forks source link

Exclude importPaths that start with ".." #2637

Open CyberShadow opened 1 year ago

CyberShadow commented 1 year ago

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.

What do you think?

github-actions[bot] commented 1 year ago

✅ 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=76s
Full build output ``` DUB version 1.31.1, built on Apr 17 2023 LDC - the LLVM D compiler (1.32.1): based on DMD v2.102.2 and LLVM 15.0.7 built with LDC - the LLVM D compiler (1.32.1) Default target: x86_64-unknown-linux-gnu Host CPU: skylake-avx512 http://dlang.org - http://wiki.dlang.org/LDC Registered Targets: aarch64 - AArch64 (little endian) aarch64_32 - AArch64 (little endian ILP32) aarch64_be - AArch64 (big endian) amdgcn - AMD GCN GPUs arm - ARM arm64 - ARM64 (little endian) arm64_32 - ARM64 (little endian ILP32) armeb - ARM (big endian) avr - Atmel AVR Microcontroller bpf - BPF (host endian) bpfeb - BPF (big endian) bpfel - BPF (little endian) hexagon - Hexagon lanai - Lanai mips - MIPS (32-bit big endian) mips64 - MIPS (64-bit big endian) mips64el - MIPS (64-bit little endian) mipsel - MIPS (32-bit little endian) msp430 - MSP430 [experimental] nvptx - NVIDIA PTX 32-bit nvptx64 - NVIDIA PTX 64-bit ppc32 - PowerPC 32 ppc32le - PowerPC 32 LE ppc64 - PowerPC 64 ppc64le - PowerPC 64 LE r600 - AMD GPUs HD2XXX-HD6XXX riscv32 - 32-bit RISC-V riscv64 - 64-bit RISC-V sparc - Sparc sparcel - Sparc LE sparcv9 - Sparc V9 systemz - SystemZ thumb - Thumb thumbeb - Thumb (big endian) ve - VE wasm32 - WebAssembly 32-bit wasm64 - WebAssembly 64-bit x86 - 32-bit X86: Pentium-Pro and above x86-64 - 64-bit X86: EM64T and AMD64 xcore - XCore Upgrading project in /home/runner/work/dub/dub/ Starting Performing "release" build using /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/ldc2 for x86_64. Building dub 1.33.0-beta.1+commit.5.gc4fa5c53: building configuration [application] source/dub/internal/dyaml/node.d(2513,9): Deprecation: scope variable `this` assigned to non-scope parameter `_param_0` calling `match` Serializing composite type Flags!(BuildRequirement) which has no serializable fields Serializing composite type Flags!(BuildOption) which has no serializable fields source/dub/dependency.d(917,18): Deprecation: scope variable `this` assigned to non-scope parameter `oth` calling `opEquals` source/dub/dependency.d(920,30): Deprecation: scope variable `this` assigned to non-scope parameter `a` calling `doCmp` source/dub/dependency.d(921,27): Deprecation: scope variable `this` assigned to non-scope parameter `b` calling `doCmp` source/dub/dependency.d(939,26): Deprecation: scope variable `this` assigned to non-scope parameter `oth` calling `opEquals` source/dub/internal/configy/Exceptions.d(130,34): Deprecation: reference to local variable `buffer` assigned to non-scope anonymous parameter source/dub/internal/configy/Exceptions.d(134,34): Deprecation: reference to local variable `buffer` assigned to non-scope anonymous parameter source/dub/internal/configy/Exceptions.d(248,27): Deprecation: `@safe` function `formatMessage` calling `formattedWrite` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/write.d(537,34): which calls `std.format.spec.FormatSpec!char.FormatSpec.writeUpToNextSpec!(void delegate(in char[]) @safe).writeUpToNextSpec` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): which would be `@system` because of: /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): scope variable `this` assigned to non-scope parameter `e` calling `put` source/dub/internal/configy/Exceptions.d(250,27): Deprecation: `@safe` function `formatMessage` calling `formattedWrite` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/write.d(537,34): which calls `std.format.spec.FormatSpec!char.FormatSpec.writeUpToNextSpec!(void delegate(in char[]) @safe).writeUpToNextSpec` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): which would be `@system` because of: /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): scope variable `this` assigned to non-scope parameter `e` calling `put` source/dub/internal/configy/Exceptions.d(283,27): Deprecation: `@safe` function `formatMessage` calling `formattedWrite` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/write.d(537,34): which calls `std.format.spec.FormatSpec!char.FormatSpec.writeUpToNextSpec!(void delegate(in char[]) @safe).writeUpToNextSpec` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): which would be `@system` because of: /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): scope variable `this` assigned to non-scope parameter `e` calling `put` source/dub/internal/configy/Exceptions.d(286,27): Deprecation: `@safe` function `formatMessage` calling `formattedWrite` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/write.d(537,34): which calls `std.format.spec.FormatSpec!char.FormatSpec.writeUpToNextSpec!(void delegate(in char[]) @safe).writeUpToNextSpec` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): which would be `@system` because of: /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): scope variable `this` assigned to non-scope parameter `e` calling `put` source/dub/internal/configy/Exceptions.d(323,31): Deprecation: `@safe` function `formatMessage` calling `formattedWrite` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/write.d(537,34): which calls `std.format.spec.FormatSpec!char.FormatSpec.writeUpToNextSpec!(void delegate(in char[]) @safe).writeUpToNextSpec` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): which would be `@system` because of: /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): scope variable `this` assigned to non-scope parameter `e` calling `put` source/dub/internal/configy/Exceptions.d(325,31): Deprecation: `@safe` function `formatMessage` calling `formattedWrite` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/write.d(537,34): which calls `std.format.spec.FormatSpec!char.FormatSpec.writeUpToNextSpec!(void delegate(in char[]) @safe).writeUpToNextSpec` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): which would be `@system` because of: /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): scope variable `this` assigned to non-scope parameter `e` calling `put` source/dub/internal/configy/Exceptions.d(332,31): Deprecation: `@safe` function `formatMessage` calling `formattedWrite` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/write.d(537,34): which calls `std.format.spec.FormatSpec!char.FormatSpec.writeUpToNextSpec!(void delegate(in char[]) @safe).writeUpToNextSpec` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): which would be `@system` because of: /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): scope variable `this` assigned to non-scope parameter `e` calling `put` source/dub/internal/configy/Exceptions.d(335,31): Deprecation: `@safe` function `formatMessage` calling `formattedWrite` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/write.d(537,34): which calls `std.format.spec.FormatSpec!char.FormatSpec.writeUpToNextSpec!(void delegate(in char[]) @safe).writeUpToNextSpec` /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): which would be `@system` because of: /opt/hostedtoolcache/dc/ldc2-1.32.1/x64/ldc2-1.32.1-linux-x86_64/bin/../import/std/format/spec.d(258,33): scope variable `this` assigned to non-scope parameter `e` calling `put` Linking dub STAT:statistics (-before, +after) STAT:executable size=5272544 bin/dub STAT:rough build time=76s ```
WebFreak001 commented 1 year ago

can you add a test for this?

CyberShadow commented 1 year ago

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.

CyberShadow commented 1 year ago

If I do chmod 000 on a directory, that does cause current Dub to fail, but I don't think that would make for a very good test. For one, it's POSIX-only; for two, that should probably be fixed to skip unreadable directories anyway, in the same way that broken symlinks are.

CyberShadow commented 1 year ago

Added it as I can't think of anything better right now, and I see the tests are POSIX-only anyway.

WebFreak001 commented 1 year ago

you still need to put in the .no_build, .no_test, .no_run files so the CI tests don't try to do those.

CyberShadow commented 1 year ago

BTW I'm seeing zero documentation on how to run the test suite. A section in the README, or a test/README.md, would be nice.

WebFreak001 commented 1 year ago

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 importPaths ".." would only scan its own directory, since it's used to make the DUB package name being used as module package name for all files within (and no source directory needed)

WebFreak001 commented 1 year ago

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.

CyberShadow commented 1 year ago

for source paths:

* https://github.com/andrey-zherikov/argparse/blob/6fca01c219828b8223b64e1c7899e1cd7c2b2eb9/examples/completion/single_main/app/dub.json#L5

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, importPaths "../source" would be valid and meaningful for a library which did this and then used locally with dub add-local.

for source files:

* https://github.com/dokutoku/dxlib-d/blob/2e539d4dfda5804fed1a4a0c29957e7dd96f7819/examples/windows/example/dub.json#L20

Makes sense.

for import paths:

* https://github.com/dlang-community/gitcompatibledubpackage/blob/master/dub.sdl#L22-L25

Ah, that one's mine! Looks like I made this a long time ago to illustrate the same problem.

I think what we should do for import paths: (and C import paths, since they are the same thing for some compilers)

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 moduleRoot declaration. So then, moduleRoot "foo" would tell Dub that a bar.d in the project root corresponds to the module foo.bar, and that the directory containing the project should be named foo, and that its parent directory should be added to the import (-I) paths; or even use the -mv compiler feature to make things align if the directory structure doesn't allow it.

WebFreak001 commented 1 year ago

Perhaps this is the wrong approach entirely, and we need something like a moduleRoot declaration. So then, moduleRoot "foo" would tell Dub that a bar.d in the project root corresponds to the module foo.bar, and that the directory containing the project should be named foo, and that its parent directory should be added to the import (-I) paths; or even use the -mv compiler feature to make things align if the directory structure doesn't allow it.

I think -mv should be avoided by the user, I more see it as something that tools could use to do special purpose stuff, such as sharing the same dependency with multiple versions in a build. Mapping module structure to the file/folder structure makes it easier to reason about what happens and explain to people how things work.

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 - in their package name.

Do you have a requirement / use-case why you need to control the parent folder/current folder relationship?

CyberShadow commented 1 year ago

I think -mv should be avoided by the user, I more see it as something that tools could use to do special purpose stuff

Yeah sorry, I meant that Dub could use -mv under the hood to implement moduleRoot.

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 - in their package name.

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 arsd_official.

Do you have a requirement / use-case why you need to control the parent folder/current folder relationship?

It's ae and projects structured like it (see that gitcompatibledubpackage project above). The structure allows using both Dub and Git submodules for dependency versioning.

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 lib_foo in a directory foo which holds modules in the lib.foo namespace, a package called lib_bar in a directory bar which holds modules in the lib.bar namespace... then, the equivalent importPath would be "../..".