Make builds able to depend on external projects #291

Baccata · 2018-04-09T17:09:45Z

Builds are now able to load external projects (also built with mill), and depend on them as if they were local submodules, using simple ammonite imports :

import $file.outer.path.build

Without this patch, external builds are compiled using pwd to create millSourcePath, disregarding the original location of the outer projects, and their sources are looked up in the current directory rather than the external one.

lihaoyi · 2018-04-09T18:07:15Z

@Baccata since this deals with build.sc files, how about a new ScriptTestSuite in the same vein as https://github.com/lihaoyi/mill/blob/master/main/test/src/mill/main/JavaCompileJarTests.scala#L7? You can set up your dummy build (doesn't need to be a Scala project) in the resources/ folder, with multiple folders with build.sc files, and then exercise the various tasks to make sure they're doing the right things in the right folders

Baccata · 2018-04-10T13:38:49Z

@lihaoyi, say I depend on an outer build under outerDir. Should the current pwd/out be used to cache the tasks from the outer build (in which case dest of the outer tasks should probably be amended to retain uniqueness), or should outerDir/out be used ?

I think I know how to implement either ways, it's just a matter of making the decision ^^

lihaoyi · 2018-04-10T15:51:55Z

@Baccata one thing that isn't clear to me is what you mean by outer; from the code snippets, it seems like the imported builds would be inside the current folder, not outside. Is your envisioned use case aggregating sub-project builds into the top-level build.sc, or is it to import builds from somewhere else on the filesystem entirely?

Would import $file.^.^.foo.build work?

It's a bit unclear to me whether a shared top-level/enclosing out/ folder (bazel style) or per-build-file out/ folder (SBT style) would be preferable.

Apart from re-wiring up dest, you would also have to re-wire millSourcePath on the imported build file. That may take a bit of fiddling and you may need to make a custom import $build hook to deal with that

Baccata · 2018-04-10T16:18:11Z

Is your envisioned use case aggregating sub-project builds into the top-level build.sc, or is it to import builds from somewhere else on the filesystem entirely?

My bad, should have been more descriptive. I don't have a definitive answer to that question. My usecase started from the former : aggregating sub-projects builds (for instance, giving the option to split code into git submodules, allowing for faster dev cycles when teams extract code as libraries). However, I realised that since ammonite allows to import external scripts from elsewhere in the filesystem, why not just allow for that ? This case encompasses the case where we only import modules defined in subdirectories.

Would import $file.^.^.foo.build work?

It actually does, with the current changes ! However, whilst writing tests, I realised that with the current modifications, ^.^.foo.build.bar (a task defined in the outer build) and bar (a task present in the current build) would have their dest collide, which breaks the "all tasks have their own unique space in FS to store stuff" guarantee.

It's a bit unclear to me whether a shared top-level/enclosing out/ folder (bazel style) or per-build-file out/ folder (SBT style) would be preferable.

Roll a dice ? 😸 If you don't have a clear preference, I think I'll have a go at implementing the former (bazel style). The only benefit I see from the latter (sbt style) is that it'd save some time when moving from one dir to another as things might already have been cached. Not sure it's worth it

Apart from re-wiring up dest, you would also have to re-wire millSourcePath on the imported build file. That may take a bit of fiddling and you may need to make a custom import $build hook to deal with that

With the current changes, millSourcePath is correctly set to the actual location of the imported build within the file system 😄(and it propagates correctly to its inner modules / tasks).

Baccata · 2018-04-10T16:31:17Z

@lihaoyi for the sake of the discussion, I pushed some WIP code which include some tests b4f041d . Particularly here

lihaoyi · 2018-04-10T17:14:25Z

I suppose the main use case of SBT-style per-subproject out/ folders is you could move around the filesystem of a large project and build things in each folder without rebuilding the world each time you cd.

This can also be accomplished by having the sub projects "discover" what the outer-most build.sc file is, e.g. what Git or Bazel do. Bazel has the problem of needing a special marker file to demarcate the "outermost" boundary, while git has the problem of sometimes noticing outer .git folders even when you don't want it to.

Maybe we can just punt on the problem for now and accept the a bit of unnecessary re-building when you cd.

w.r.t. filesystem-collisions, there are a few approaches we could take:

We could prohibit the case where you both have a foo/build.sc and a object foo in the root build.sc. For example, using a special import $build.foo hook that injects a import $file.foo.build; final val foo = build into the current build.sc file. Not sure how this would work for outer ^ builds, but it should suffice for inner builds
We could come up with a scheme to disambiguate the two. e.g. inner builds could be put in some dest/inner-build/ folder, since modules can't have dashes in their names (we already make use of this for e.g. the mill-worker or mill-profile misc files)
We could just punt it and hope people don't make an object and a inner-folder-with-build-file of the same name. This is the Python foo.py/foo/__init__.py strategy

Baccata · 2018-04-10T17:37:44Z

Maybe we can just punt on the problem for now and accept the a bit of unnecessary re-building when you cd.

Agreed

We could come up with a scheme to disambiguate the two. e.g. inner builds could be put in some dest/inner-build/ folder, since modules can't have dashes in their names (we already make use of this for e.g. the mill-worker or mill-profile misc files)

I think I'll roll with this. Adding another magic import that is not fully orthogonal with $file does not feel right to me, and disambiguating looks easy enough.

Thanks for the answers !

Builds are now able to load external projects and depend on them as if they were local submodules. `import $file.external.path.build`

* Calling modules loaded from external directories "Foreign" to avoid conflicting with the already existing concept of "ExternalModule". * Amended the way `dest` is computed for foreign modules * Added tests to check that the source paths and dest are as expected * Added a test to show that local modules do not conflict with foreign modules when they are named the same

Baccata · 2018-04-13T07:54:43Z

core/src/mill/eval/Evaluator.scala

@@ -30,7 +30,6 @@ case class Labelled[T](task: NamedTask[T],
 }
 case class Evaluator[T](home: Path,
                        outPath: Path,
-                        externalOutPath: Path,


this parameter was always equal to outPath (except in one test), so I took the liberty to remove. I think I could add a external-modules segment in out in the new destSegments conditioned by the module/task being "external". Thoughts ?

We re-used externalOutPaths in tests to speed up the test suite; otherwise re-initializing the Zinc incremental compiler made them take forever. Do you have a good reason to believe this is no longer the case?

Ah ! Fair enough, reverted there 1607f78

Baccata · 2018-04-13T07:55:28Z

core/src/mill/eval/Evaluator.scala

@@ -191,6 +191,27 @@ case class Evaluator[T](home: Path,
        }
    }
  }
+
+  def destSegments(labelledTask : Labelled[_]) : Segments = {


This is the core of the PR I guess. This makes sure that "foreign" modules do not conflict with local modules

Baccata · 2018-04-13T07:56:36Z

main/test/resources/examples/foreign/project/build.sc

@@ -0,0 +1,82 @@
+import $file.^.outer.build
+import $file.inner.build
+


this is the main test, it checks the consistency of sourcePaths and dest

Baccata · 2018-04-13T07:57:03Z

main/test/resources/examples/foreign/project/build.sc

+  assertPaths(^.outer.build.sub.sub.selfPath(), thisPath / up / 'outer / 'sub / 'sub)
+}
+
+def checkOuterInnerPaths = T {


checking that importing a foreign build which imports a foreign build works as expected

Baccata · 2018-04-13T07:57:40Z

main/test/src/mill/util/ScriptTestSuite.scala

@@ -8,28 +8,29 @@ import utest._
 abstract class ScriptTestSuite(fork: Boolean) extends TestSuite{


Add to tweak this a bit to accommodate the new tests. Changes are not breaking

Baccata · 2018-04-13T07:59:17Z

core/src/mill/define/Ctx.scala

               fileName: String){
 }

 object Ctx{
  case class External(value: Boolean)
+  case class Foreign(value : Boolean)


Had to choose a name for the concept "builds that originate from other directories", and External was already taken. Obviously I'm not a native English speaker, so perhaps there's a term that would suit the concept better.

Baccata · 2018-04-13T12:46:52Z

.appveyor.yml

@@ -46,6 +46,7 @@ build_script:
      C:\%CYGWIN_DIR%\bin\bash -lc "curl -Lo /usr/local/bin/mill %MILL_URL%" &&
      C:\%CYGWIN_DIR%\bin\bash -lc 'sed -i '"'"'0,/-cp "\$0"/{s/-cp "\$0"/-cp `cygpath -w "\$0"`/}; 0,/-cp "\$0"/{s/-cp "\$0"/-cp `cygpath -w "\$0"`/}'"'"' /usr/local/bin/mill' &&
      C:\%CYGWIN_DIR%\bin\bash -lc 'chmod +x /usr/local/bin/mill' &&
-      C:\%CYGWIN_DIR%\bin\bash -lc "cd /cygdrive/c/mill && mill -i all main.test scalajslib.test")
+      C:\%CYGWIN_DIR%\bin\bash -lc "cd /cygdrive/c/mill && mill -i all __.publishLocal release" &&


the cygwin job was not bootstrapping before testing, leading the tests to fail

Can you explain why this is necessary? I don't see any reason the changes I've reviewed would cause bootstrapping issues

Sure. I momentarily switched output stream to System.out in failing test to understand what was happening.

As far as I understand, without bootstrapping the foreign0 param was not defined in the version of mill used to compile this wrapping code

This started happened locally as well after I rebased the changes against the master (I had been out of sync for ~10 days I think)

Running the unit tests via mill all main.test should use the freshly-compiled version of Mill; the fact that it's including foreign0 in the code wrapper is evidence of this. Thus I do not see why that freshly-compiled Mill would not be able to handle foreign0 during compilation.

Furthermore, the Travis build ran the foreign module tests also without bootstrapping, but successfully.

Can you dig into this a bit deeper? We should figure out what is going on here rather than just working around the issue.

Baccata · 2018-04-13T12:48:54Z

@lihaoyi, I think I've reached a satisfying state for this. If you're happy with it, I'll proceed and add some documentation

lihaoyi · 2018-04-13T16:28:06Z

Apart from one comment looks ok i guess

* Add documentation for foreign-modules

Baccata · 2018-04-18T07:50:41Z

@lihaoyi, regarding the failed appveyor build when using the cygwin environment, here's what I found so far :

The notable differences between the cygwin one and (either of) the others :
1. the tests are not run using a bootstrapped mill
2. JDK 9 is used
Switched to JDK 9 locally , and found out the problem is reproducible (mill main.test).
Wondered why JavaCompileJarTests was green in the same build, found that it does not execute when java version is 9 . When the condition is commented, these tests fail for the same reason
Here's a minimal example that show the issue occurring when running the tests in JDK 9.

I'm happy to investigate further, but this shows the PR is not at cause. I'd like to make the CI green again, which I can make by either re-adding the bootstrapping line in the cygwin env, or disabling the tests like in JavaCompileJarTests. So my next questions would be :

Any reason why bootstrapping does not occur in the cygwin build, or was it just an oversight ?
I assume you had a good reason for disabling JavaCompileJarTests in java9. What would that be ?

lihaoyi · 2018-04-18T15:51:49Z

@Baccata we wanted to run our test suite with a mix of bootstrapped and non-bootstraped runs, to make sure it works on both.

I don't remember the reason for disabling JavaCompileJarTests in java9; it probably just slipped in during the chaos of getting java9 working.

I think your options are to either figure out what's causing the non-bootstrapped test to fail, or disabling it in java 9. I don't think turning on bootstrapping temporarily to get them working is viable: that just pushes the problem down the line to when it fails some time in future.

If you disable it in java9, open an issue to write down what you know about the problem.

See com-lihaoyi#302

lihaoyi · 2018-04-18T21:35:58Z

Looks good to me

Baccata force-pushed the allow-imports-of-external-builds branch from 661fb6b to d0fa222 Compare April 10, 2018 13:31

Baccata added 2 commits April 13, 2018 09:51

Make builds able to depend on external projects

6cf77f1

Builds are now able to load external projects and depend on them as if they were local submodules. `import $file.external.path.build`

Baccata force-pushed the allow-imports-of-external-builds branch from b4f041d to 60d1cdc Compare April 13, 2018 07:51

Baccata commented Apr 13, 2018

View reviewed changes

Baccata added 2 commits April 13, 2018 14:05

WIP windows build fail

4216a72

Added bootstrapping step in CYGWIN CI job

6d2b1f3

Baccata force-pushed the allow-imports-of-external-builds branch from b49d657 to 6d2b1f3 Compare April 13, 2018 12:07

Baccata commented Apr 13, 2018

View reviewed changes

* Revert externalOutPath deletion

1607f78

* Add documentation for foreign-modules

Baccata force-pushed the allow-imports-of-external-builds branch from 3945ce4 to 1607f78 Compare April 14, 2018 10:07

reverting appveyor changes

40dac00

Baccata mentioned this pull request Apr 18, 2018

Non-bootstrapped tests might fail when building mill against java9 #302

Closed

Disabling Foreign modules tests against Java9

305e2a1

See com-lihaoyi#302

lihaoyi merged commit 7898368 into com-lihaoyi:master Apr 18, 2018

Baccata deleted the allow-imports-of-external-builds branch April 20, 2018 09:29

lefou added this to the 0.2.1 milestone May 2, 2019

lihaoyi mentioned this pull request Jun 27, 2023

Make per-folder build.sc files a first-class feature to support large projects #2637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make builds able to depend on external projects #291

Make builds able to depend on external projects #291

Baccata commented Apr 9, 2018 •

edited

Loading

lihaoyi commented Apr 9, 2018

Baccata commented Apr 10, 2018

lihaoyi commented Apr 10, 2018 •

edited

Loading

Baccata commented Apr 10, 2018 •

edited

Loading

Baccata commented Apr 10, 2018 •

edited

Loading

lihaoyi commented Apr 10, 2018

Baccata commented Apr 10, 2018

Baccata Apr 13, 2018

lihaoyi Apr 13, 2018

Baccata Apr 14, 2018

Baccata Apr 13, 2018

Baccata Apr 13, 2018

Baccata Apr 13, 2018

Baccata Apr 13, 2018

Baccata Apr 13, 2018

Baccata Apr 13, 2018

lihaoyi Apr 13, 2018

Baccata Apr 14, 2018 •

edited

Loading

lihaoyi Apr 14, 2018

Baccata Apr 15, 2018

Baccata commented Apr 13, 2018

lihaoyi commented Apr 13, 2018

Baccata commented Apr 18, 2018 •

edited

Loading

lihaoyi commented Apr 18, 2018

lihaoyi commented Apr 18, 2018

		@@ -0,0 +1,82 @@
		import $file.^.outer.build
		import $file.inner.build

		@@ -8,28 +8,29 @@ import utest._
		abstract class ScriptTestSuite(fork: Boolean) extends TestSuite{

Make builds able to depend on external projects #291

Make builds able to depend on external projects #291

Conversation

Baccata commented Apr 9, 2018 • edited Loading

lihaoyi commented Apr 9, 2018

Baccata commented Apr 10, 2018

lihaoyi commented Apr 10, 2018 • edited Loading

Baccata commented Apr 10, 2018 • edited Loading

Baccata commented Apr 10, 2018 • edited Loading

lihaoyi commented Apr 10, 2018

Baccata commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baccata Apr 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baccata commented Apr 13, 2018

lihaoyi commented Apr 13, 2018

Baccata commented Apr 18, 2018 • edited Loading

lihaoyi commented Apr 18, 2018

lihaoyi commented Apr 18, 2018

Baccata commented Apr 9, 2018 •

edited

Loading

lihaoyi commented Apr 10, 2018 •

edited

Loading

Baccata commented Apr 10, 2018 •

edited

Loading

Baccata commented Apr 10, 2018 •

edited

Loading

Baccata Apr 14, 2018 •

edited

Loading

Baccata commented Apr 18, 2018 •

edited

Loading