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.

Sign up for GitHub

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

Jump to bottom

Don't include resources into compileClasspath. #1811

Merged
merged 3 commits into from
Dec 10, 2022
Merged

Don't include resources into compileClasspath. #1811

merged 3 commits into from
Dec 10, 2022

Conversation

vic
Copy link
Contributor

@vic vic commented Apr 2, 2022 •
edited by lefou
Loading

Instead we now have a compileResources that is part of compileClasspath.

Fixes #1807

@lefou lefou added the compat-breaker This PR is good to merge, but breaks compatibility and needs to wait till we prepare a major release label Apr 4, 2022
@lefou
Copy link
Member

lefou commented Apr 4, 2022 •
edited
Loading

This looks ok so far, but this need to wait until the next major version (probably 0.11), as it changes the dependencies between targets which is an issue for downstream plugins.

To get CI green, some tests need to be adapted.

@lefou
Copy link
Member

lefou commented Apr 4, 2022 •
edited
Loading

This looks ok so far, but this need to wait until the next major version (probably 0.11), as it changes the dependencies between targets which is an issue for downstream plugins.

We have no clear rule when to start development on next major. Until now, we just bumped the version when needed. So that might be also the case for this PR. But as we now take compatibility serious, we should avoid too many major bumps in a short time to remove stress from downstream projects like external Mill plugins.

@lefou
Copy link
Member

lefou commented Apr 5, 2022

This is the error in CI:

X mill.scalalib.bsp.BspModuleTests.bspCompileClasspath.single module.dependent module 362ms 
  utest.AssertionError: #2: relResults == expected,
  relResults: Seq[os.FilePath] = Vector(/home/runner/work/mill/mill/target/worksources/mill/scalalib/bsp/BspModuleTests/MultiBase/HelloBsp/compile-resources, /home/runner/work/mill/mill/target/worksources/mill/scalalib/bsp/BspModuleTests/MultiBase/HelloBsp2/resources, /home/runner/work/mill/mill/target/workspace/mill/scalalib/bsp/BspModuleTests/eval/bspCompileClasspath/single module/dependent module/HelloBsp/compile.dest/classes, logback-classic-1.1.10.jar, logback-core-1.1.10.jar, scala-library-2.13.8.jar, slf4j-api-1.7.34.jar)
  expected: Seq[os.FilePath] = List(/home/runner/work/mill/mill/target/worksources/mill/scalalib/bsp/BspModuleTests/MultiBase/HelloBsp/resources, /home/runner/work/mill/mill/target/worksources/mill/scalalib/bsp/BspModuleTests/MultiBase/HelloBsp2/resources, /home/runner/work/mill/mill/target/workspace/mill/scalalib/bsp/BspModuleTests/eval/bspCompileClasspath/single module/dependent module/HelloBsp/compile.dest/classes, logback-classic-1.1.10.jar, logback-core-1.1.10.jar, scala-library-2.13.8.jar, slf4j-api-1.7.34.jar)
    utest.asserts.Asserts$.assertImpl(Asserts.scala:30)
    mill.scalalib.bsp.BspModuleTests$.$anonfun$tests$12(BspModuleTests.scala:81)
    mill.scalalib.bsp.BspModuleTests$.$anonfun$tests$12$adapted(BspModuleTests.scala:59)
    mill.scalalib.bsp.BspModuleTests$.workspaceTest(BspModuleTests.scala:37)
    mill.scalalib.bsp.BspModuleTests$.$anonfun$tests$11(BspModuleTests.scala:40)

You need to reflect the changed resource directory name in the expected result.

@vic
Copy link
Contributor Author

vic commented Apr 5, 2022

@lefou thanks will fix that. I saw a lot of ci failures and didn't still had enough time to find which of them were caused by my change. thanks for pointing out. any particular suite you recommend me to run ?

@lefou
Copy link
Member

lefou commented Apr 5, 2022 •
edited
Loading

As you touch scalalib it's a good idea to run scalalib.test, at least. It's perfectly ok to push and just review what the CI reports. Besides of some sporadic CI failures (which I typically review and restart from time to time), all test must pass.

@lefou lefou mentioned this pull request Apr 7, 2022
Closed
@lefou lefou added the next-major This change is a candidate for a next major release label Jul 25, 2022
@lefou lefou self-assigned this Aug 30, 2022
@lefou lefou added this to the next major milestone Sep 27, 2022
@lefou lefou marked this pull request as ready for review December 8, 2022 18:23
@lefou lefou merged commit 98e35cd into com-lihaoyi:main Dec 10, 2022
@lefou lefou modified the milestones: next major, 0.11.0-M1 Dec 10, 2022
@lefou
Copy link
Member

lefou commented Dec 10, 2022

We finally merged it. Thanks @vic for this contribution and your patience!

@lihaoyi lihaoyi mentioned this pull request Apr 7, 2023
Merged
lihaoyi added a commit that referenced this pull request Apr 8, 2023
@lihaoyi
Move BuildInfo from sources into resources, defer use of resources du…
d9cf409 
…ring compilation (#2425)

Instead of compiling the buildinfo values into the source code, we only
compile the buildinfo keys into the source code and store the buildinfo
values in the resources, to be loaded at runtime. This greatly reduces
the amount of time spent compiling things when buildinfo changes. In
this repo, some rough tests indicate that it makes the second `-i
installLocal` after a one-line change in the `LICENSE` file drop from
50s to 14s of runtime

`mill-profile.json` Before:


[mill-profile.txt](https://github.com/com-lihaoyi/mill/files/11177836/mill-profile.txt)


`mill-profile.json` After:


[mill-profile.txt](https://github.com/com-lihaoyi/mill/files/11177798/mill-profile.txt)

Notes:

1. To really benefit from this, I had to make `compileClasspath` depend
only on upstream module's `compileClasspath()`/`compile().classes`, so
it doesn't end up depending on their runtime `resources()` and forcing
unnecessary recompiles when the buildinfo resources change. This is
probably a change we want to do anyway, given the direction set by
#1811

2. I consolidated the BuildInfo implementation with
`contrib/buildinfo/`.

3. Extended `contrib/buildinfo/` to support Java modules, which we
dogfood in `main.client`.

4. The old behavior of statically compiling the values into the binary
is still available under `def buildInfoStaticCompiled = true`, for
anyone who needs it. That includes anyone using `BuildInfo` in Scala.js,
which doesn't support JVM resources by default

5. For now, I copy-pasted the implementation into the root `build.sc`
file, and updated `mill-bootstrap.patch` to remove it and make use of
the `mill-contrib` version. When we re-bootstrap Mill on latest main, we
can remove the copy-pasta

6. `contrib/buildinfo/` should be mostly source compatible with the
previous implementation, except that I made `buildInfoPackageName` a
required field to encourage the best practice of not putting code in the
empty package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lefou lefou

Labels
compat-breaker This PR is good to merge, but breaks compatibility and needs to wait till we prepare a major release next-major This change is a candidate for a next major release
Projects
None yet
Milestone
0.11.0-M1
Development

Successfully merging this pull request may close these issues.

Java/Scala: Improve resources handling
2 participants
@vic @lefou