-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
More Gradle: now with makeshift release management! Ready for v3.0.0 #3871
Conversation
Hooray Jenkins reported success with all tests good! |
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.
More or less looking fine. Some minor changes requested, and long-term we should try to get away from too much logic in the buil file but rather derive as much information from other things present (e.g., version from git tags, where to release to from version, ...)
@@ -17,16 +17,45 @@ publishing { | |||
maven { | |||
name = 'TerasologyOrg' | |||
|
|||
def repoViaEnv = System.getenv()["PUBLISH_REPO"] | |||
if (rootProject.hasProperty("publishRepo")) { |
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'm wondering whether we should start to scope the properties in the gradle builds, similar to what most plugins to. This would become terasology.publishRepo
then...
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.
Had not thought about that before. Even in a plain build file, rather than a plugin? I've been curious if some of our logic might go better in a plugin, where it would make all kinds of sense, since then you'd probably use neato little blocks to configure it.
config/gradle/publish.gradle
Outdated
if (gitBranch != null && gitBranch != "" && gitBranch.equals("master")) { | ||
// Okay we're in an environment where a branch name is set to 'master' so we might be doing a release | ||
// This is the funny part. Modules aren't ready globally to accept master branch == release, so check a prop that defaults to bypass | ||
if (project.hasProperty("bypassModuleReleaseManagement")) { | ||
if (bypassModuleReleaseManagement == "true") { | ||
println "Release management not enabled for " + project.name + ", using snapshot repo despite building 'master' branch" | ||
deducedPublishRepo += snapshotRepoFragment | ||
} else { | ||
println "Release management *is* enabled for " + project.name + ", using release repo since building 'master' branch" | ||
deducedPublishRepo += releaseRepoFragment | ||
} | ||
} else { | ||
println "We're working on a 'master' branch with defaults so using the release publish repo" | ||
deducedPublishRepo += releaseRepoFragment | ||
} | ||
} else { | ||
// No master branch so we for sure are just doing snapshots | ||
println "We're not working with a branch name set that's 'master' so assuming we're publishing snapshots" | ||
deducedPublishRepo += "-snapshot-local" | ||
} |
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.
The whole logic in here boils down to just this, doesn't it?
if (isMaster(maybeGitBranch) && !shouldBypassModuleRelease()) {
deducedPublishRepo += "-release-local"
} else {
deducedPublishRepo += "-snapshot-local"
}
I think with the two additional functions this code is really readable, and the nesting is reduced.
def isMaster(gitBranch) {
return gitBranch != null && gitBranch != "" && gitBranch.equals("master");
}
def shouldBypassModuleRelease() {
return project.hasProperty("bypassModuleReleaseManagement") &&
bypassModuleReleaseManagement == "true";
}
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.
This would loose a couple of console lines, but overall the structure in the build file is more comprehensible imho.
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.
Thank you! I had even more logic madness in there while trying to find a solution that would cover all the scenarios, this was actually an already boiled down version, but closing in on 2 am was not letting me see clearly in shrinking it further. This is way cleaner! Don't need the logging now that it tests out fine :-)
} else { | ||
// No master branch so we for sure are just doing snapshots | ||
println "We're not working with a branch name set that's 'master' so assuming we're publishing snapshots" | ||
deducedPublishRepo += "-snapshot-local" |
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.
deducedPublishRepo += "-snapshot-local" | |
deducedPublishRepo += snapshotRepoFragment |
// Temporary workaround: default modules to bypass release management: master branch builds will still make snapshot builds for the snapshot repo | ||
// If a module actually wants release management simply include `"isReleaseManaged" : true` in module.txt - this is needed for the engine repo embedded modules | ||
// One option would be to slowly convert modulespace to default to a `develop` + `master` setup living in harmony with associated automation/github tweaks | ||
// Alternatively one more round of refactoring could switch to Git tags, a single `master` branch, and possible other things to help match snaps/PR builds somehow? |
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.
Heavyly in favor of git tags, as most modules don't have complicated branching going on anyways. But you're right, should not be done in this PR, and needs some proper thoughtwork to figure out details and edge cases...
// This won't work without additionally doing constant version bumps (perhaps via Git tags) - but too much work to switch around all modules at once | ||
// Complicating things more the use of publish.gradle to centralize logic means modules and engine bits are treated the same, yet we need to vary modules | ||
// Temporary workaround: default modules to bypass release management: master branch builds will still make snapshot builds for the snapshot repo | ||
// If a module actually wants release management simply include `"isReleaseManaged" : true` in module.txt - this is needed for the engine repo embedded modules |
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 don't like the fact that the generic module.txt
now holds information specific to our build system. As it is not officially documented anywhere, I guess its not much of a deal. Nevertheless, I would like to get rid of it asap.
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.
Yep, it is a workaround for sure. Lets me get through this big release without an even bigger overhaul. It can be an undocced feature likely never to be used in module land as we can hopefully finish improving things soon.
if (moduleConfig.isReleaseManaged && env.BRANCH_NAME == "master") { | ||
// This is mildly awkward since we need to bypass by default, yet if release management is on (true) then we set the bypass to false .. | ||
ext.bypassModuleReleaseManagement = false | ||
if (version.contains("-SNAPSHOT")) { |
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.
if (version.contains("-SNAPSHOT")) { | |
if (version.endsWith("-SNAPSHOT")) { |
out.withStyle(Style.FailureHeader).println("WARNING: Module " + project.name + " is explicitly versioned as a snapshot in module.txt, please remove '-SNAPSHOT'") | ||
} | ||
} else { | ||
if (!version.contains("-SNAPSHOT")) { |
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.
if (!version.contains("-SNAPSHOT")) { | |
if (!version.endsWith("-SNAPSHOT")) { |
import static org.gradle.internal.logging.text.StyledTextOutput.Style | ||
|
||
// The only case in which we make module non-snapshots is if release management is enabled and BRANCH_NAME is "master" | ||
if (moduleConfig.isReleaseManaged && env.BRANCH_NAME == "master") { |
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.
see above
templates/build.gradle
Outdated
if (moduleConfig.isReleaseManaged && env.BRANCH_NAME == "master") { | ||
// This is mildly awkward since we need to bypass by default, yet if release management is on (true) then we set the bypass to false .. | ||
ext.bypassModuleReleaseManagement = false | ||
if (version.contains("-SNAPSHOT")) { |
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.
if (version.contains("-SNAPSHOT")) { | |
if (version.endsWith("-SNAPSHOT")) { |
templates/build.gradle
Outdated
out.withStyle(Style.FailureHeader).println("WARNING: Module " + project.name + " is explicitly versioned as a snapshot in module.txt, please remove '-SNAPSHOT'") | ||
} | ||
} else { | ||
if (!version.contains("-SNAPSHOT")) { |
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.
if (!version.contains("-SNAPSHOT")) { | |
if (!version.endsWith("-SNAPSHOT")) { |
Hooray Jenkins reported success with all tests good! |
Thanks a lot for the fresh eyes on this! Helped a lot, cleaner now, tests out still, hopefully looks better now. Gotta fix the Javadoc thing soon then will merge and do the release 👍 |
def subprojectPath = ':' + subprojectName | ||
def subproject = project(subprojectPath) | ||
subproject.projectDir = possibleSubprojectDir | ||
if (!possibleSubprojectDir.name.startsWith(".")) { |
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.
What about a general approach like this (in the future): https://medium.com/@ungesehn/use-gitignore-for-gradle-task-excludes-e5d011e99f71
.settings
should be gitignores regardless of where in the file tree...
… branch name Change env var Console logging issue Fixy
Avoids having to manage -SNAPSHOT in code, instead it is deduced from the environment if a branch name is set (Jenkins) If branch is master (and for modules if release management is opted into) then artifacts will be published to a release repo, otherwise to a snapshot repo Retains support for varying the target publish org in Artifactory so "terasology" vs "nanoware" for instance.
…detection (can happen in some IDEs)
Hooray Jenkins reported success with all tests good! |
Fixed the Javadoc issue, validated with the Nanoware jobs etc, and merged! Initial engine develop job ran fine |
This ate my entire weekend, although just the spare time between GSOC proposal review, baby maintenance, and so on. Up way too late so will be brief ™️ - had imagined I might merge this straight to develop then PR to master for the v3.0.0 release, but too late and now too big.
Realized I needed to be able to vary how artifacts get published, either as releases or snapshots to Artifactory. This used to be done by having two separate jobs in the legacy Jenkins, one tied to
master
that made releases, one tied todevelop
that made snapshots. The actual version numbers were modified by hand as part of doing a release (take off-SNAPSHOT
, push tomaster
, bump version and reattach-SNAPSHOT
...)New Jenkins has a multi-branch pipeline job. Shiny. But it was publishing everything as snapshots, even pull requests getting built, meaning at any point in time a module build could run against some engine artifact that came out of an unmerged PR. Not great.
Okay not too bad. Quick conditional to vary publishing by branch just for now. Oh, and really then I might as well just leave the version numbers without
-SNAPSHOT
and attach that dynamically if we're indevelop
! Awesome bonus. Wait, but then what about modules. They use the same sharedpublish.gradle
to keep things standard. That means they'll publish releases when building frommaster
branches 🤔Alright, too much work to make
develop
branches for everything, default them, have devs do both branches as a standard... besides we might be able to do better like with Git tags (ohai @skaldarnar) instead of branches, and hey now checks are in place in code to where we could probably just look at tags instead of branches. But this needs to work right now.Awkward release management to the rescue! We'll default modules to not have release management enabled, meaning even while building
master
branches unless"isReleaseManaged" : true
is added tomodule.txt
the new stuff won't trigger and we'll still just make snapshots. And the embedded modules with the engine will get that set, but nobody else yet. It isn't super pretty, but ... it works.For any reviewers: The actual change in
build.gradle
for modules is minimal, I just had to move some things around. This has been tested with the Nanoware job line in Jenkins and repo line in Artifactory (which also took some extra effort to get right)Bonus: The code will now gently (or angrily, if red text is angry) warn about modules that still use
-SNAPSHOT
in their own version inmodule.txt
, so we can eventually clean that out (or just move the version out of code into tags). Thanks @sin3point14 for that!Outstanding
develop
build of the engine. More work to do and scenarios to satisfy - but we'll get there. Shout out to @PS-Soundwave