-
-
Notifications
You must be signed in to change notification settings - Fork 427
Support nested build files with relaxed package naming #6504
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
base: main
Are you sure you want to change the base?
Conversation
b16122b to
d60cd78
Compare
|
Hi @Emin017, I have seen your PR. Great work. And I have tested on my branch, but got a dependency error: My repo is at https://github.com/project-inochi/misc-ips/tree/mill-subproject In fact, I am also trying to resolve this, but got lots of cycle dependency. |
@inochisa Could you make this repo public? That way, we can see your specific configuration and debug more effectively. |
|
@Emin017 My fault, now it is opened. |
|
@inochisa I took a look at your repository structure, and I have a general understanding of how the issue is occurring. We may still need to modify the configuration mechanism for searching subfolders and the assertion check mechanism in mill. Previously, I only considered the following check and usage mechanism: // build.mill
package build
import mill.*, scalalib.*
trait BarModule extends build_.deps.foo.FooModule // extends trait from deps/foo/build.mill// deps/package.mill
package build.deps// deps/foo/build.mill
package build
import mill.*, scalalib.*
trait FooModule extends ScalaModule {
def scalaVersion = "2.13.16"
}
object `package` extends FooModuleHowever, looking at it now, I realize that I didn't fully consider this aspect. I'll think about making adjustments to this part. (We might also need more relaxed check rules.) |
Agreed.
I think it is possible to remove this (at least in my repository). I added it as mill always complaint it is not a package.
I also consider this, but I found it is not nature to do such a thing. Using internal
In this example, the SpinalHDL has its own build lib. This is not resolved in a right way, as mill will generate |
@inochisa I think Mill’s CodeGen will rewrite package name prefixes, which should prevent loops from occurring (at least in this case). Could you explain in more detail when the loop you mentioned for package imports would arise? |
Signed-off-by: Qiming Chu <[email protected]>
Signed-off-by: Qiming Chu <[email protected]>
Signed-off-by: Qiming Chu <[email protected]>
Signed-off-by: Emin <[email protected]>
Signed-off-by: Emin <[email protected]>
Signed-off-by: Qiming Chu <[email protected]>
Signed-off-by: Qiming Chu <[email protected]>
Signed-off-by: Qiming Chu <[email protected]>
Signed-off-by: Qiming Chu <[email protected]>
d60cd78 to
23d984a
Compare
|
@inochisa I think I roughly understand what you mean. I’ve pushed some new commits, and you can try it based on that. The new support I added includes:
// deps/foo/build.mill
// This is build.mill in subdir, but also import trait from its subdir
package build
import build.foo.Bar
trait fooModule extends Bar// deps/foo/bar/package.mill
package build.bar
trait Bar {
// Do something here
}I’m not sure whether there are other aspects I may have overlooked. Please feel free to point them out after testing :) |
|
Thanks for your update @Emin017 Mostly, I think it is works, but I got an weird error for the build. I guess it may not related to this build and it is a new problem? I have found a similar bug here at sbt/sbt#7777. @lihaoyi, could you take a look for it? I have attached the log below. https://gist.github.com/inochisa/aa362832d7736df7b4d427ac26841a0a |
|
@inochisa can you try wrapping everything in an explicit |
|
@lihaoyi Yeah, by adding this, it is resolved, and I got an error related to this PR. It seems like this is related to the package generation. https://gist.github.com/inochisa/ad1353ea0d2b2a28e72f5ba2fc84fc90 |
@inochisa Could you sync your latest configuration to the repository? That way, I can reproduce the issue and debug. |
- Nested build.mill: allow empty package (standalone project) or relaxed match - Nested package.mill: require package declaration, but allow relaxed match - Root build.mill: allow empty or exact match only Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
@Emin017 I have updated the repository by force push. |
Signed-off-by: Emin <[email protected]>
The local `build` object generated inside `object package_` was causing
Scala 3 compiler errors ("missing outer accessor") when user code in
`abstract class package_` referenced it. This happened because the
nested object shadowed the imported `build` alias, breaking outer
reference resolution.
Solution: Generate the `build` object at package level instead of inside
`object package_`. This eliminates the shadowing issue while maintaining
the functionality for nested build.mill files to reference sibling
modules via `import build.foo.Bar`.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Signed-off-by: Emin <[email protected]>
Signed-off-by: Emin <[email protected]>
|
@inochisa Can you try the latest commit? I think the previous error should have been fixed. ❯ ./mill dist.run misc-ips/ -i compile -explain-cyclic
8850] dist.run
Mill version SNAPSHOT is different than configured for this directory!
Configured version is 1.0.6 (/home/Emin/Workbench/mill/misc-ips/.mill-version)
============================== compile -explain-cyclic ==============================
build.mill-64] compile compiling 10 Scala sources to out/mill-build/compile.dest/classes ...
build.mill-64] [warn] ext/SpinalHDL/project/Version.mill:5:25
build.mill-64] import scala.collection.JavaConverters._
build.mill-64] ^^^^^^^^^^^^^^
build.mill-64] object JavaConverters in package scala.collection is deprecated since 2.13.0: Use `scala.jdk.CollectionConverters` instead
build.mill-64]
build.mill-64] [warn] one warning found
build.mill-64] done compiling
69/69, completed] ========================== compile -explain-cyclic ========================== 8s
[error] Cannot resolve compile. Try `mill resolve _`, `mill resolve __.compile` to see what's available, or `mill __.compile` to run all `compile` tasks
8850/8850, 1 failed, completed] ============================== dist.run misc-ips/ -i compile -explain-cyclic ============================== 9s
8850] [error] dist.run dist.run failed with an exception. Result of /home/Emin/Workbench/mill/out/dist/launcher.dest/run…: 1 |
|
@Emin017 Confirmed # clear && ./mill -i dist.run xxx/misc-ips -i hw.runMain io.inochisa.gen.clock.ClockDetectedVerilog
8850] dist.run
Mill version SNAPSHOT is different than configured for this directory!
Configured version is 1.0.6 (xxx/misc-ips/.mill-version)
build.mill-64] compile compiling 1 Scala source to out/mill-build/compile.dest/classes ...
build.mill-64] done compiling
build.mill-64] compiling 5 Scala sources to out/mill-build/compile.dest/classes ...
build.mill-64] [warn] ext/SpinalHDL/project/Version.mill:5:25
build.mill-64] import scala.collection.JavaConverters._
build.mill-64] ^^^^^^^^^^^^^^
build.mill-64] object JavaConverters in package scala.collection is deprecated since 2.13.0: Use `scala.jdk.CollectionConverters` instead
build.mill-64]
build.mill-64] [warn] one warning found
build.mill-64] done compiling
378] hw.runMain
[Runtime] SpinalHDL dev git head : 0a2bd7bc67e6ce86c1231386c18cfd8c216919d1
[Runtime] JVM max memory : 30208.0MiB
[Runtime] Current date : 2026.01.09 12:19:05
[Progress] at 0.000 : Elaborate components
[Progress] at 0.016 : PhaseCreateComponent
[Progress] at 0.284 : PhaseDummy
[Progress] at 0.284 : Checks and transforms
[Progress] at 0.284 : PhaseMemBlackBoxingDefault
[Progress] at 0.288 : PhaseDeviceSpecifics
[Progress] at 0.288 : PhaseApplyIoDefault
[Progress] at 0.290 : PhaseNameNodesByReflection
[Progress] at 0.292 : PhaseCollectAndNameEnum
[Progress] at 0.296 : PhaseCheckIoBundle
[Progress] at 0.298 : PhaseCheckHierarchy
[Progress] at 0.301 : PhaseAnalog
[Progress] at 0.303 : PhaseNextifyReg
[Progress] at 0.304 : PhaseRemoveUselessStuff
[Progress] at 0.312 : PhaseRemoveIntermediateUnnameds
[Progress] at 0.316 : PhasePullClockDomains
[Progress] at 0.319 : PhaseInferEnumEncodings
[Progress] at 0.327 : PhaseInferWidth
[Progress] at 0.329 : PhaseNormalizeNodeInputs
[Progress] at 0.333 : PhaseRemoveIntermediateUnnameds
[Progress] at 0.333 : PhaseSimplifyNodes
[Progress] at 0.337 : PhaseCompletSwitchCases
[Progress] at 0.337 : PhaseRemoveUselessStuff
[Progress] at 0.338 : PhaseRemoveIntermediateUnnameds
[Progress] at 0.339 : PhaseCheck_noLatchNoOverride
[Progress] at 0.348 : PhaseCheck_noRegisterAsLatch
[Progress] at 0.349 : PhaseCheckCombinationalLoops
[Progress] at 0.353 : PhaseCheckCrossClock
[Progress] at 0.357 : PhasePropagateNames
[Progress] at 0.368 : PhaseObfuscate
[Progress] at 0.368 : PhaseAllocateNames
[Progress] at 0.374 : PhaseDevice
[Progress] at 0.375 : PhaseGetInfoRTL
[Progress] at 0.376 : PhaseDummy
[Progress] at 0.377 : Generate Verilog to gen
[Progress] at 0.377 : PhaseVerilog
[Done] at 0.417
378/378, completed] ============================== hw.runMain io.inochisa.gen.clock.ClockDetectedVerilog ============================== 19s
8850/8850, completed] ============================== dist.run xxx/misc-ips -i hw.runMain io.inochisa.gen.clock.ClockDetectedVerilog ============================== 25s
|
|
@Emin017 I can also confirmed the change for adding Module for every object is not needed anymore. |
Signed-off-by: Emin <[email protected]>
|
CI Status link: runs/20844288189 The relevant tests have passed, and most tests on CI have also passed. The only failure is the EDIT: I have also updated the PR description. |
The |
Thanks, it is useless. And I just did a copy paste from the original test, so please ignore this thing. |
|
I think this looks great, could you include the example page in the relevant |
Signed-off-by: Emin <[email protected]>
| // | ||
| // === Example Project | ||
| // The `FooModule` trait is defined in the nested `deps/foo/build.mill` file, and we can reference it | ||
| // using the hierarchical path `build_.deps.foo.FooModule` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this work without the build_ prefix? In normal build.mill/package.mill files you generally never need to write build_, and we should try to provide the same experience here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code generation logic to support importing build.deps.foo.FooModule (instead of build_.deps.foo.FooModule).
Previously generated file
// Generated by Mill ...
package build_.deps.foo
import build_.{package_ => build}
import MillMiscInfo.*
import _root_.mill.util.TokenReaders.given
import _root_.mill.api.JsonFormatters.given
object package_ extends package_ {
export build_.deps.foo.*
}
//SOURCECODE_ORIGINAL_FILE_PATH=.../deps/foo/build.mill
//SOURCECODE_ORIGINAL_CODE_START_MARKER
import mill.*, scalalib.*abstract class package_
extends _root_.mill.api.internal.SubfolderModule(build.millDiscover), FooModuleNewly generated file
// Generated by Mill ...
package build_.deps.foo
import build_.{package_ => build}
import MillMiscInfo.*
import _root_.mill.util.TokenReaders.given
import _root_.mill.api.JsonFormatters.given
object _defs {
import mill.*, scalalib.*
trait FooModule extends ScalaModule {
def scalaVersion = myScalaVersion
}
}
export _defs._
object package_ extends package_ {
export _defs._
export build_.deps.foo.*
}
//SOURCECODE_ORIGINAL_FILE_PATH=.../deps/foo/build.mill
//SOURCECODE_ORIGINAL_CODE_START_MARKER
import mill.*, scalalib.*
trait FooModule extends ScalaModule {
def scalaVersion = myScalaVersion
}
abstract class package_
extends _root_.mill.api.internal.SubfolderModule(_root_.build_.package_.millDiscover), FooModuleOld code generation flow
The previous code generation pipeline now will produce invalid output such as:
import mill.*, scalalib.*abstract class package_which is a syntax error.
New code generation flow
The new pipeline works as follows:
- Detect whether there is any code between
lastImportEndandpackageObjectStart- This required extending
MillScalaParserto support the detection.
- This required extending
- If such code exists:
- Create a
_defswrapper:
- Create a
baseHeader
+ object _defs { top-level definitions }
+ export _defs._
+ object package_ { export _defs._ }
+ wrapperTail
- During concatenation:
- Only keep imports in
importsPrefix - Append
rewrittenScriptCode.substring(packageObjectStart)
- Only keep imports in
This results in:
import mill.*, scalalib.*
trait FooModule ...
abstract class package_ ...
which is syntactically correct.
However, these implementations have changes in many places across the repository, and I’m not sure whether they might introduce other issues. By comparison, supporting imports only from build_.deps.foo would be much simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/Emin017/mill/actions/runs/20895535833
I think this is now fully implemented, so I'll convert this PR from draft to ready for review.
Signed-off-by: Emin <[email protected]>
Signed-off-by: Qiming Chu <[email protected]>
Try to keep binary compatibility. Signed-off-by: Qiming Chu <[email protected]>
a2d08d2 to
160fb9f
Compare
Signed-off-by: Qiming Chu <[email protected]>
2035a61 to
9f74828
Compare
Signed-off-by: Qiming Chu <[email protected]>
Signed-off-by: Qiming Chu <[email protected]>
9f74828 to
ca84ac9
Compare
This is an attempt to support #5091
allNestedBuildFileNamestoCodeGenConstantsto include bothpackage.*andbuild.*variants, enabling recognition of both types as nested build files.CodeGen.scalaandFileImportGraph.scalato useallNestedBuildFileNamesfor identifying nested build files and allowing relaxed package declarations for these files.Relaxed Package Declarations
This update introduces relaxed package naming for nested mill build files, enabling the same build definition file to be used in different scenarios (as an independent project or as a reusable submodule).
Commit: 0636620
package build,package build.foopackage build,package build.foo,package build.barpackage build.deps,package build.foo.barpackage build.helpersRelaxed Match Definition
A relaxed match occurs when the import path is either:
build*(i.e.,package build)package foo, and we can importfoo.bar.baz)This means that any package name in the form of
package build*is valid, and we can also expect an import path likefoo.bar.bazwhen we havepackage foo.Usage Example
We can now import build configuration files from subdirectories:
If the subdirectory imports nested build files, this is also supported:
I believe that if this feature is merged, it will help smooth the migration of several open-source projects (such as Chisel, SpinalHDL, rocket-chip, and Xiangshan) to the new version of Mill, as they currently rely on this functionality in the older version of Mill.