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

PublishModule deletes moduleDeps created by other traits #2082

Closed
lolgab opened this issue Oct 23, 2022 · 1 comment · Fixed by #2083
Closed

PublishModule deletes moduleDeps created by other traits #2082

lolgab opened this issue Oct 23, 2022 · 1 comment · Fixed by #2083
Labels
bug The issue represents an bug
Milestone

Comments

@lolgab
Copy link
Member

lolgab commented Oct 23, 2022

PublshModule has the following code:

override def moduleDeps: Seq[PublishModule] = Seq.empty[PublishModule]

override def moduleDeps: Seq[PublishModule] = Seq.empty[PublishModule]

which creates a very strange behaviour when a base trait manipulates the moduleDeps. If you add with PublishModule the dependencies added previously by other traits disappear.

Proposed solution

If it's important that all the moduleDeps are PublishModules, it should instead iterate over the moduleDeps and make sure that all the moduleDeps are proper PublishModules, and then cast the result if the requirement passes, but it shouldn't delete any existing moduleDeps otherwise.

@lefou
Copy link
Member

lefou commented Oct 24, 2022

PublshModule has the following code:

override def moduleDeps: Seq[PublishModule] = Seq.empty[PublishModule]

override def moduleDeps: Seq[PublishModule] = Seq.empty[PublishModule]

which creates a very strange behaviour when a base trait manipulates the moduleDeps. If you add with PublishModule the dependencies added previously by other traits disappear.

Proposed solution

If it's important that all the moduleDeps are PublishModules, it should instead iterate over the moduleDeps and make sure that all the moduleDeps are proper PublishModules, and then cast the result if the requirement passes, but it shouldn't delete any existing moduleDeps otherwise.

I agree. We should call super.moduleDeps and fail with a sensible error message if any of them is not a PublishModule

@lefou lefou added the bug The issue represents an bug label Oct 24, 2022
lolgab added a commit to lolgab/mill that referenced this issue Oct 24, 2022
lefou pushed a commit that referenced this issue Oct 24, 2022
Instead of overriding `moduleDeps` to be `PublishModule`s we iterate
over the existing `moduleDeps` and we cast to `PublishModule`s if they
are of that type, otherwise we throw an exception with an error message
to fix the problem.

Fixes #2082 

Pull request: #2083
@lefou lefou added this to the after 0.10.8 milestone Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue represents an bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants