Skip to content

GEODE-8239 - Add Gradle config to generate manifest file#5292

Closed
yozaner1324 wants to merge 1 commit intoapache:feature/GEODE-8067from
yozaner1324:GEODE-8239
Closed

GEODE-8239 - Add Gradle config to generate manifest file#5292
yozaner1324 wants to merge 1 commit intoapache:feature/GEODE-8067from
yozaner1324:GEODE-8239

Conversation

@yozaner1324
Copy link
Contributor

Add Gradle config to generate manifest file containing 'Class-Path' and 'Dependent-Modules' attributes.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

@kohlmu-pivotal kohlmu-pivotal requested review from a user and kohlmu-pivotal and removed request for a user June 23, 2020 23:58
module5WithManifestCompileOnly(sourceSets.test.output)

module1WithManifestCompile('org.springframework:spring-core')
module1WithManifestCompile('com.google.guava:guava:29.0-jre')
Copy link

Choose a reason for hiding this comment

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

Check in buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy to see if this version is already defined

module1WithManifestCompile('org.springframework:spring-core')
module1WithManifestCompile('com.google.guava:guava:29.0-jre')

testCompile(project(":geode-core"))
Copy link

Choose a reason for hiding this comment

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

Prefer testImplementation over testCompile

// to avoid conflicts over redefining certain configurations.
// Evaluation as to whether java-platform should be applied at all is left to GEODE-6611.
apply plugin: 'java-platform'
// This anti-pattern is a workaround -- java-platform must be applied before java or java-library
Copy link

Choose a reason for hiding this comment

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

Please restore this to a two-space indent, and change style in a different PR.


org.gradle.internal.http.socketTimeout=120000
org.gradle.internal.http.connectionTimeout=120000
org.gradle.workers.max = 3
Copy link

Choose a reason for hiding this comment

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

Really not sure about this change. I think it will slow down builds a ton.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah it will.. but also kills local machines if it uses every core for this... I'd proven it out on a 24 core box, with less "max workers" it actually worked faster than max cores.
BUT, that said, this is local changes and can be left off PR.

// log4j-core has an annotation processor that is passed on the compile-classpath via geode-core and others.
// Fix Gradle warning here until we clean up our own classpath
annotationProcessor 'org.apache.logging.log4j:log4j-core:' + DependencyConstraints.get('log4j.version')
// log4j-core has an annotation processor that is passed on the compile-classpath via geode-core and others.
Copy link

Choose a reason for hiding this comment

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

See above about indentation.

}
javac.options.incremental = true
javac.options.fork = true
javac.options.forkOptions.with({
Copy link

Choose a reason for hiding this comment

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

See above about indentation.


manifest {
attributes(
"Manifest-Version": "1.0",
Copy link

Choose a reason for hiding this comment

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

Would love if we could merge the new attributes in, instead of overwriting. Any chance of that, before we merge to develop ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll look into this before we submit PR to merge into develop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments