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

Package in local-packages not picked up after moving package into new directory structure #2691

Closed
jaapgeurts opened this issue Aug 24, 2023 · 4 comments · Fixed by #2833
Closed

Comments

@jaapgeurts
Copy link

System information

  • dub version: dub 1.33.0
  • OS Platform and distribution: (Linux opensuse tumbleweed (up to date))
  • compiler version LDC - the LLVM D compiler (1.32.0), based on DMD v2.102.2 and LLVM 15.0.7, built with LDC - the LLVM D compiler (1.32.0), Default target: x86_64-suse-linux

Bug Description

Dub issues a warning that the current package is at the wrong path. After moving the package to the suggested directory, dub doesn't see the package anymore.
Example: Warning Package at path '/home/alice/src/dlang-packages/ddbus/' should be under '/home/alice/src/dlang-packages/foo/$VERSION/foo'

After move the package into the suggested directory, dub doesn't see the package anymore.

How to reproduce?

  1. Add a local package directory dub add-path <path>
  2. Check the $HOME/.dub/packages/local-packages.json and make sure the path is listed.
  3. Add a package into the directory. (using the old structure without the version numbers)
  4. Issue dub list
  5. Notice that the package is listed and that dub issues a warning: Package at path <path>/foo should be under <path>/$VERSION/foo
  6. Move the package to the suggested directory(under a version number),
  7. Issue dub list
  8. Notice that the warning is gone but the package is not listed.

When trying to build a project it will not pick up the dependency.

Expected Behavior

Dub does see the package after moving it into the suggested directory.

@s-ludwig
Copy link
Member

This is a bogus warning introduced by #2610. It should really only apply to the package cache path(s) (e.g. ~/.dub/packages/) - dub add-path should not be affected in any way. The logic there needs to be changed to first make the isManagedPath check and then perform the legacy package search with that warning. For unmanaged paths it should just search directly, with no warning.

@Geod24
Copy link
Member

Geod24 commented Jan 4, 2024

@s-ludwig : Actually, it's not a bogus warning, but it is a bug introduced in #2610 .
The idea is to move towards storing the version outside the recipe file, both for performance and to avoid modifying the downloaded zip as we currently do. However, the condition excludes non managed paths.
I get that his could be considered as making dub add-path harder to use, but it actually makes the expectations much clearer and aligned with managed paths, which IMO makes it easier to use. For example, what happens if you currently have packages in custom paths that have no version field ?

Geod24 added a commit to Geod24/dub that referenced this issue Jan 8, 2024
Currently, we issue a warning asking people to migrate, but the migrated
path is not actually recognized. It was suggested in the issue to ignore
this warning for add-path. However, as add-path is a rather low-level
use, I think it is better to impose the same condition for managed and
unmanaged path, in order to be able to provide the same guarantees.
For example, once we move to reading version from the path,
we can provide the same speedups for add-path users.
We can also make sure that add-path users don't have surprises when
dealing with packages that expect a certain structure outside their
directory, such as arsd and ae.
Some tests were updated with the new path, however, as many will likely
be rewritten with the new test framework, and a lot of tests are affected,
not all of them were updated, providing coverage for the old and new
code alike.
@Geod24
Copy link
Member

Geod24 commented Jan 8, 2024

Fix: #2780

Geod24 added a commit to Geod24/dub that referenced this issue Jan 8, 2024
Currently, we issue a warning asking people to migrate, but the migrated
path is not actually recognized. It was suggested in the issue to ignore
this warning for add-path. However, as add-path is a rather low-level
use, I think it is better to impose the same condition for managed and
unmanaged path, in order to be able to provide the same guarantees.
For example, once we move to reading version from the path,
we can provide the same speedups for add-path users.
We can also make sure that add-path users don't have surprises when
dealing with packages that expect a certain structure outside their
directory, such as arsd and ae.
Some tests were updated with the new path, however, as many will likely
be rewritten with the new test framework, and a lot of tests are affected,
not all of them were updated, providing coverage for the old and new
code alike.
Geod24 added a commit to Geod24/dub that referenced this issue Jan 10, 2024
Currently, we issue a warning asking people to migrate, but the migrated
path is not actually recognized. It was suggested in the issue to ignore
this warning for add-path. However, as add-path is a rather low-level
use, I think it is better to impose the same condition for managed and
unmanaged path, in order to be able to provide the same guarantees.
For example, once we move to reading version from the path,
we can provide the same speedups for add-path users.
We can also make sure that add-path users don't have surprises when
dealing with packages that expect a certain structure outside their
directory, such as arsd and ae.
Some tests were updated with the new path, however, as many will likely
be rewritten with the new test framework, and a lot of tests are affected,
not all of them were updated, providing coverage for the old and new
code alike.
Geod24 added a commit that referenced this issue Jan 10, 2024
Currently, we issue a warning asking people to migrate, but the migrated
path is not actually recognized. It was suggested in the issue to ignore
this warning for add-path. However, as add-path is a rather low-level
use, I think it is better to impose the same condition for managed and
unmanaged path, in order to be able to provide the same guarantees.
For example, once we move to reading version from the path,
we can provide the same speedups for add-path users.
We can also make sure that add-path users don't have surprises when
dealing with packages that expect a certain structure outside their
directory, such as arsd and ae.
Some tests were updated with the new path, however, as many will likely
be rewritten with the new test framework, and a lot of tests are affected,
not all of them were updated, providing coverage for the old and new
code alike.
@ibuclaw ibuclaw mentioned this issue Jan 28, 2024
s-ludwig added a commit that referenced this issue Feb 16, 2024
This fixes issue #2691 properly by just silencing the bogus warnings and scanning as always. There is no reason in expecting or supporting the same sub directory structure as for managed paths.
s-ludwig added a commit that referenced this issue Feb 16, 2024
s-ludwig added a commit that referenced this issue Feb 16, 2024
Reverts to the scanning/warning behavior that was in place before the warnings for the new package directory structure were implemented.
@s-ludwig
Copy link
Member

@s-ludwig : Actually, it's not a bogus warning, but it is a bug introduced in #2610 . The idea is to move towards storing the version outside the recipe file, both for performance and to avoid modifying the downloaded zip as we currently do. However, the condition excludes non managed paths. I get that his could be considered as making dub add-path harder to use, but it actually makes the expectations much clearer and aligned with managed paths, which IMO makes it easier to use. For example, what happens if you currently have packages in custom paths that have no version field ?

Just to also mention it here. This makes absolutely no sense. The main use case for dub add-path is to add your "dev" folder where you have the working copies of the packages you develop. Nobody in their right mind would organize those with such a scheme. Performance improvements simply must be achieved in a different way.

s-ludwig added a commit that referenced this issue Feb 16, 2024
s-ludwig added a commit that referenced this issue Feb 16, 2024
Reverts to the scanning/warning behavior that was in place before the warnings for the new package directory structure were implemented.
s-ludwig added a commit that referenced this issue Feb 16, 2024
Reverts to the scanning/warning behavior that was in place before the warnings for the new package directory structure were implemented.
Geod24 added a commit that referenced this issue Feb 17, 2024
Properly fix issue #2691 by reverting to the original scanning behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants