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

Fix/transient deps #4343

Merged
merged 8 commits into from
Jan 3, 2021
Merged

Fix/transient deps #4343

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b8d7f2a
chore: use short-form copyright
keturn Dec 28, 2020
5cfcf10
chore (build): refactor lambda to named function
keturn Dec 28, 2020
ef739b4
fix (build): use gradle to resolve all runtime classpath for facades
keturn Dec 28, 2020
e821433
feat (build): make :modules:clean clean each module
keturn Dec 28, 2020
1f325e7
Merge remote-tracking branch 'origin/develop' into fix/transient-deps
keturn Dec 28, 2020
c15b055
Merge remote-tracking branch 'origin/develop' into fix/transient-deps
keturn Dec 29, 2020
1f59ade
Merge remote-tracking branch 'origin/develop' into fix/transient-deps
keturn Jan 2, 2021
f383cb1
chore (build): the dependency on modules is for local dev builds
keturn Jan 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,14 +190,10 @@ def terasologyModules() {
}

// Helpers that do magic things after having dependencies attached below
task moduleClasses
task moduleJars

// This magically makes everything work - without this the desired module projects returned have no tasks :-(
gradle.projectsEvaluated {
// Note how "classes" may indirectly trigger "jar" for module dependencies of modules (module compile dependency)
moduleClasses.dependsOn(terasologyModules().classes)

// This makes it work for a full jar task
moduleJars.dependsOn(terasologyModules().jar)
}
Expand Down
5 changes: 3 additions & 2 deletions facades/PC/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ dependencies {

// TODO: Consider whether we can move the CR dependency back here from the engine, where it is referenced from the main menu
implementation(group = "org.terasology.crashreporter", name = "cr-terasology", version = "4.1.0")

runtimeOnly(platform(project(":modules")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth adding a quick comment here? I'm not even sure what "platform" even means here, although I can figure what the overall meaning is

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can do

}

// Instructions for packaging a jar file for the PC facade
Expand All @@ -113,11 +115,10 @@ configurations {
}

// Used for all game configs.
val commonConfigure : JavaExec.()-> Unit = {
fun JavaExec.commonConfigure() {
group = "terasology run"

dependsOn(":extractNatives")
dependsOn(":moduleClasses")
dependsOn("classes")

// Run arguments
Expand Down
3 changes: 2 additions & 1 deletion facades/TeraEd/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ sourceSets {
dependencies {
implementation project(':engine')
implementation group: 'org.reflections', name: 'reflections', version: '0.9.10'

runtimeOnly(platform(project(":modules")))
}

application {
Expand All @@ -46,7 +48,6 @@ task editor(type:JavaExec) {

// Dependencies: natives + all modules & the PC facade itself (which will trigger the engine)
dependsOn rootProject.extractNatives
dependsOn rootProject.moduleClasses
dependsOn classes

// Run arguments
Expand Down
25 changes: 25 additions & 0 deletions modules/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Copyright 2020 The Terasology Foundation
// SPDX-License-Identifier: Apache-2.0

plugins {
`java-platform`
}

javaPlatform {
allowDependencies()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here I fully expect that this works, but is allowing dependencies not a default state of some sort? That seems odd.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the case! By default, gradle's java-platform is a thing that specifies version constraints, but consumers can depend on the platform without depending on everything in it.

that scenario probably looks more like:

dependencies {
    implementation(platform("com.example.foo:foo-framework:3"))
    implementation("com.example.foo:foo-core")
    implementation("com.example.foo:foo-http-server")
}

to say "I need these things from Foo Framework v3, make sure all versions are consistent with what they've published as v3 of that platform."

In order to say this platform depends on things we have to explicitly enable set this flag.

}

dependencies {
// This platform depends on each of its subprojects.
subprojects {
runtime(this)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figure this builds the runtime classpath so all the module jars are available to the engine. this can be used to refer to a whole project level in Gradle, so to say?

This reminds me of that old idea to make the PC facade have a compile dependency on all the modules, to have them for sure get built when you run the game. Although at the cost of not being able to launch without every single module working, and maybe at a risk to get weird dependencies written into the binaries published to Artifactory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is given by the closest enclosing scope; it's the project that subprojects is iterating through. (the hints added by IntelliJ make this clearer when reading it in the IDE)

make the PC facade have a compile dependency on all the modules

Yes, that's exactly what is happening when the facade depends on the platform.

at the cost of not being able to launch without every single module working

I think we already sacrificed that ability a while ago?

Test plan:

  • groovyw recurse JoshariasSurvival
  • add some compilation error to EdibileFlora's FillingModifierSystem
  • gradlew game --continue
    • :modules:EdibleFlora:compileJava fails

I think because the game task depends on the moduleClasses task.

I haven't yet come across a gradle way to always attempt to rebuild dependency subprojects if their sources changed but also soft-fail and try to run subsequent steps despite that. You can --continue, but I think that only makes it keep going on independent parts of the dependency tree, not excuse a missing dependency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get weird dependencies written into the binaries published to Artifactory.

yes, if you publish a jar of the PC facade while you have subprojects present in /modules/, that would include things we probably don't want included.

Is that a thing that happens? Does anything publish facade that's not in a clean workspace made explicitly for that purpose?

If so, then yeah, we'll have to add more separation so this local workspace state isn't confused with what we want to publish to the world. I hadn't imagined it was used that way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gradle will always hit the issues, yep, IntelliJ has had a tendency in the past to skip errors if they're in something the run config doesn't care about. I'm not sure about that now since we're having some other weird issues there from time to time :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you publish a jar of the PC facade ...

I didn't see this comment earlier while posting my own for some reason. No, that isn't done anywhere at the moment. And any sort of test-publishing we might do locally surely should target something like the Nanoware test repos in Artifactory.

It is good to know though, in case we get creative and weird in the future. One idea that floated around was to build modules in an actual full engine workspace, rather than the minified single-module workspace we run in Jenkins right now. Even that in theory shouldn't hit it - since that should just publish the module, not the facade.

So - not worried about it. May be worth highlighting with a comment perhaps :-)

}
}

// Allows using :modules:clean as a shortcut for running clean in each module.
tasks.named("clean").configure {
val cleanPlatform = this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again understanding what this does, but cleanPlatform throws me off naming-wise. Is it almost like "cleanPlatform" would really be the task name? But since you want :modules:clean to work then the task name just remains "clean"

Would something like cleanAllModules and cleanAllModules.dependsOn(... do the same thing but be clearer in naming?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleanPlatform here is a local variable so I can use it below after this has been shadowed by the subproject. It's not renaming any tasks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, I got that part. Thus the "almost like" 😁

Like in the abstract this class could as well have been called "cleanPlatform" or "cleanAllModules" - plain "cleanPlatform" just throws me off as a variable name the way it gets used, but that's very minor and probably just me 👍

subprojects {
cleanPlatform.dependsOn(this.tasks.named("clean"))
}
}
17 changes: 2 additions & 15 deletions modules/subprojects.settings.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,5 @@
/*
* Copyright 2020 MovingBlocks
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
// Copyright 2020 The Terasology Foundation
// SPDX-License-Identifier: Apache-2.0

// This magically allows subdirs in this subproject to themselves become sub-subprojects in a proper tree structure
new File(rootDir, 'modules').eachDir { possibleSubprojectDir ->
Expand Down