fix: Add iosSimulatorArm64, allow overriding publication validation
#117
Conversation
…de for local development
This reverts commit e423548.
|
|
||
| // Only publish publications with the configured group from JReleaser or everything if JReleaser group is not configured | ||
| // Only publish publications with the configured group from JReleaser, or everything if JReleaser group is not configured | ||
| val publishGroupName = System.getenv(EnvironmentVariables.GROUP_ID) | ||
| shouldPublish = shouldPublish && (publishGroupName == null || publication.groupId.startsWith(publishGroupName)) | ||
| shouldPublish = shouldPublish && (publishGroupName == null || publication.groupId.equals(publishGroupName, ignoreCase = true)) | ||
|
|
||
| val overrideGroupNameValidation = project.extra.getOrNull<String>(OVERRIDE_KOTLIN_NATIVE_GROUP_NAME_VALIDATION) == "true" | ||
| if (overrideGroupNameValidation) println("Overriding group name validation for Kotlin/Native publications") | ||
|
|
||
| // Validate publication name is allowed to be published | ||
| shouldPublish = shouldPublish && | ||
| ( | ||
| ALLOWED_PUBLICATION_NAMES.any { publication.name.equals(it, ignoreCase = true) } || | ||
| // standard publication | ||
| (KOTLIN_NATIVE_PUBLICATION_NAMES.any { publication.name.equals(it, ignoreCase = true) } && KOTLIN_NATIVE_PROJECT_NAMES.any { project.name.equals(it, ignoreCase = true) }) // Kotlin/Native publication | ||
| (ALLOWED_KOTLIN_NATIVE_PUBLICATION_NAMES.any { publication.name.equals(it, ignoreCase = true) } && (overrideGroupNameValidation || ALLOWED_KOTLIN_NATIVE_GROUP_NAMES.any { publication.groupId.equals(it, ignoreCase = true) })) // Kotlin/Native publication | ||
| ) |
There was a problem hiding this comment.
Question: I'm a little confused at how this works. If you're trying to override and publish smithy-kotlin, won't shouldPublish get set to false on L392 because publication.groupId won't equal publishGroupName? And if so, won't the shouldPublish = shouldPublish && ... logic on L398 short-circuit and skip evaluating overrideGroupNameValidation?
There was a problem hiding this comment.
That logic on L392 is only used by our Catapult release (dependent on JReleaser environment variable). That needs to be cleaned up separately. In local development the publishGroupName == null part is true and short-circuits
There was a problem hiding this comment.
Cleaned up this logic, should be ready for another review now
| if (!requiredVariables.all { System.getenv(it).isNotBlank() }) { | ||
| println("Skipping JReleaser configuration, missing one or more required environment variables: ${requiredVariables.joinToString()}") | ||
| return | ||
| } |
There was a problem hiding this comment.
Question: Why did this switch from logger.info to println?
There was a problem hiding this comment.
Yeah I noticed this too. logger is only available within the context of a Gradle task
There was a problem hiding this comment.
Sorry logger was available there, but not in isAvailableForPublication
| jar = false | ||
| verifyPom = false // jreleaser doesn't understand toml packaging | ||
| jar = false // Version catalogs don't produce a JAR | ||
| verifyPom = false // JReleaser fails when processing <packaging>toml</packaging> tags: `Unknown packaging: toml` |
There was a problem hiding this comment.
Do we need to set verifyPom to false in the artifactOverride as well as in the artifacts block?
There was a problem hiding this comment.
I think we can leave verifyPom enabled for most artifacts. If it causes issues, then we can look at disabling it
| ) | ||
|
|
||
| if (!requiredVariables.all { System.getenv(it).isNotBlank() }) { | ||
| logger.info("Skipping JReleaser configuration, missing one or more required environment variables: ${requiredVariables.joinToString()}") |
There was a problem hiding this comment.
Just a preference, I liked how we were able to see exactly which env vars were missing before
There was a problem hiding this comment.
I think it may have been useful for debugging while adding JReleaser, but it's very noisy for everyday builds. We would have five lines printed saying there are missing environment variables for every build
| deploy { | ||
| maven { | ||
| mavenCentral { | ||
|
|
There was a problem hiding this comment.
What is this block for? Will it be filled out later?
There was a problem hiding this comment.
Sorry that was included prematurely, I just pushed a commit to fix configuration for klib artifacts
3 participants
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.