yshrsmz / BuildKonfig

BuildConfig for Kotlin Multiplatform Project
Apache License 2.0
782 stars 32 forks source link

Merge buildKonfig for native targets #17

Closed kuuuurt closed 4 years ago

kuuuurt commented 4 years ago

For iOS, we have to create a target for the simulator (iosX64) and the device (iosArm64/iosArm32). I'm merging them through:

val iosX64 = iosX64()
val iosArm64 = iosArm64("ios")
android()

configure(listOf(iosX64, iosArm64)) {
    binaries {
        framework(frameworkName) {
            baseName = frameworkName
        }
    }
}

val commonMain by sourceSets.getting {
    dependencies {
        ...
    }
}

val iosX64Main by sourceSets.getting
val iosMain by sourceSets.getting {
    iosX64Main.dependsOn(this)
    dependencies {
        ...
    }
}

Problem is, buildkonfig generates for both the targets but since iosX64 is dependent on iosArm64, it produces an redeclaration error. Is there any way we can ignore some of the targets?

yshrsmz commented 4 years ago

What I am doing now in my current project is changing iOS target depending on a value passed from Xcode(or command line).

When I build iOS framework, I pass a property like this

./gradlew copyFramework -Pkotlin.target=iosArm64

provided kotlin.target decides which target Gradle should configure, thus we don't need to manage two sourceSets.

kotlin {
    def iosTargetName = project.findProperty("kotlin.target") ?: "iosX64"
    def iosTarget = presets.getByName(iosTargetName)

    android()
    targetFromPreset(iosTarget, "ios") {
        binaries {
            framework([DEBUG, RELEASE])
        }
    }

    sourceSets {
        androidMain {}
        iosMain {}
    }
}
yshrsmz commented 4 years ago

Well since 1.3.60, ios dsl actually creates both iosArm64 and iosX64 targets and common sourcesets for these two.

https://kotlinlang.org/docs/reference/building-mpp-with-gradle.html#target-shortcuts

I need to check the behavior

kuuuurt commented 4 years ago

I use the switching mechanism as well. I'm also trying out 1.3.60 but I'm having problems with other libraries.

glootjofo commented 4 years ago

I also have this issue, however it is not easy to solve with a switch flag, because I'm building, and need to build all iOS targets for a static library to include in a fatLibrary.

Is there anything I can do to solve this issue myself? Or could this be fixed by adding a flag, to only generate the key value for common?

yshrsmz commented 4 years ago

@glootjofo do you have any specific implementation for each iOS target?

If there's no target-specific code, the code in the comment above is the only way for now.

./gradlew linkReleaseIosFramework -Pkotlin.target=iosX64
./gradlew linkReleaseIosFramework -Pkotlin.target=iosArm64
./gradlew linkReleaseIosFramework -Pkotlin.target=iosArm32

could this be fixed by adding a flag, to only generate the key value for common?

Yes, probably we need this feature.

glootjofo commented 4 years ago

Sadly there is target-specific code. In my case, having only the generated key-value in common would solve my specific case. Because I only need git information that is shared across all targets.

Is it something that could be added with ease?

yshrsmz commented 4 years ago

@kuuuurt Does adding an option to generate a common BuildKonfig object(no expect-actual) solve your problem?

I'm leaning towards adding generateCommonObject flag(looking for better flag name).

@glootjofo

Is it something that could be added with ease?

It depends on how we implement, but shouldn't be that difficult

yshrsmz commented 4 years ago

I came up with two idea

or

Though I prefer the first one

glootjofo commented 4 years ago

First one works for me. For now I have created a task that does this without BuildKonfig, but obviously i'll swap back if you get this feature in.

tasks.create("createCommonBuildConfig") {
    group = "gloot"
    val BuildConfigBody = """internal object BuildConfig {
  val SDK_VERSION: String = "${getGitTagFromCurrentBranch(project)}"
}"""

    val packageName = randomPackageName

    val buildConfigDirectory = File(buildDir, "buildconfig")
    buildConfigDirectory.deleteRecursively()
    buildConfigDirectory.mkdir()

    val commonMain = File(buildConfigDirectory, "commonMain")
    commonMain.mkdir()

    var previousCreatedFolder = commonMain

    packageName.split(".").forEach {
        val createdFolder = File(previousCreatedFolder, it)
        createdFolder.mkdir()
        previousCreatedFolder = createdFolder
    }

    val buildConfigSourceFile = File(previousCreatedFolder, "BuildConfig.kt")
    buildConfigSourceFile.createNewFile()
    buildConfigSourceFile.writeText(BuildConfigBody, Charsets.UTF_8)

    val mppExtension = project.extensions.getByType(KotlinMultiplatformExtension::class.java)
    mppExtension.sourceSets.getByName("commonMain")
        .kotlin.srcDir(commonMain.toRelativeString(project.projectDir))

}
kuuuurt commented 4 years ago

Sorry for being away for a while.

The first idea would be nice! Though I think a warning isn't needed. I think of it as just overriding the value or maybe adding more platform-specific values.

I'm just thinking about what if these common BuildKonfigs are just used among a subset of the targets. For example:

Common
- Target 1
- Target 2
- Target 3
- Target 4

IosCommon
- Target 3
- Target 4

I can have a:

defaultConfigs {
    // For Target 1, 2, 3, and 4
}

But I'd have to write the common configs for iOS individually:

targetConfigs("target3") {
    ... 
}

targetConfigs("target4") {
    ...
}

What do you think about a:

defaultConfigs {
    ...
    subDefaultConfigs(listOf("myTarget3", "myTarget4")) {
        ... 
    }
}

targetConfigs("target3") {
    // Only Target 3 specific 
}

targetConfigs("target4") {
    // Only Target 4 specific 
}
yshrsmz commented 4 years ago

@kuuuurt

Ok, first let me clarify your example as target in your example has two meanings.

targetConfigs API is for setting target-specific values. In this context, target means platform, such as Android and iOS.

targetConfigs(and defaultConfigs) optionally takes a string value, which I call flavor in BuildKonfig.(in respect for Android)

flavor is here to switch values depending on your environment, such as development and production(or something like paid and free).

So in your example you wrote

Common
- Target 1
- Target 2
- Target 3
- Target 4

IosCommon
- Target 3
- Target 4

but this is actually

Common
- Flavor 1
- Flavor 2
- Flavor 3
- Flavor 4

IosCommon
- Flavor 3
- Flavor 4

right?

So based on above, what you need is the ability to de-duplicate values that are common in a subset of your flavors. Am I right?

yshrsmz commented 4 years ago

I confirmed that the configuration below is valid

// set values common in some of the targets
targetConfigs {
  ["ios", "android"].each { key ->
    create(key) {
      buildConfigField "STRING", "test", "test string"
    }
  }
}

// set values common in "dev" and "stg" flavor
["dev", "stg"].each { flavor ->
  targetConfigs(flavor) {
    ios {
      buildConfigField "STRING", "commonkey", "commonvalue"
    }
  }
}

// you can set additional value for one of the flavors
// multiple calls to same target config will result in merged BuildKonfig
targetConfigs("dev") {
  ios {
    buildConfigField "STRING", "iosdevkey", "iosdevvalue"
  }
}
kuuuurt commented 4 years ago

Oh. In that case, that's good then!

Now, it's just the merging thing.

Have you verified the behavior on 1.3.60? If it works, then we might just have to upgrade to it.

yshrsmz commented 4 years ago

I've changed my mind and went this way

  • The plugin will generate a common object if no targetConfigs is specified

18

Let me know what you think. I'm gonna merge this next Tuesday or Wednesday.

@kuuuurt @glootjofo

yshrsmz commented 4 years ago

I've quickly tested ios dsl in 1.3.61 and it seems to indeed create a common sourceset for both iosArm64 and iosX64.

When I run generatBuildKonfig task it creates BuildKonfig for iosArm64 and iosX64 and seems to compile fine for both targets.

kuuuurt commented 4 years ago

In that case, I guess this one's solved in 1.3.61!

kuuuurt commented 4 years ago

Closing this issue as it's solved in 1.3.60+