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

Jump to bottom

Add native targets #23

Merged
merged 8 commits into from
May 18, 2023
Merged

Add native targets #23

merged 8 commits into from
May 18, 2023

Conversation

luca992
Copy link
Contributor

@luca992 luca992 commented Apr 21, 2022 •
edited
Loading

I added all native targets that kassert (the only limiting dependency of KHex) supports which are:

val darwinTargets = arrayOf(
    "macosX64", "macosArm64",
    "iosArm64", "iosX64", "iosSimulatorArm64",
    "tvosArm64", "tvosX64", "tvosSimulatorArm64",
    "watchosArm32", "watchosArm64", "watchosX64", "watchosSimulatorArm64",
)
val linuxTargets = arrayOf("linuxX64", "linuxArm64")
val mingwTargets = arrayOf("mingwX64")

luca992 added a commit to eqoty-labs/secretk that referenced this pull request Apr 26, 2022
@luca992
temporarily include repository until multiplatform PRs for dependenci…
076d0d4 
…es are accepted

KHash PR: komputing/KHash#23
kotlin-bip39: Electric-Coin-Company/kotlin-bip39#14
@luca992 luca992 mentioned this pull request Apr 29, 2022
Closed
@luca992 luca992 mentioned this pull request May 17, 2023
Open
ligi
ligi requested changes May 17, 2023
View reviewed changes
buildSrc/src/main/kotlin/mpp-module.gradle.kts Outdated
}
sourceSets {
commonTest {
dependencies {
implementation(kotlin("test-common"))
implementation(kotlin("test-annotations-common"))
implementation("com.github.komputing.khex:extensions:${Versions.khex}")
implementation("KHex:extensions:${Versions.khex}")
Copy link
Member

Choose a reason for hiding this comment

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

this breaks the build for me:

FAILURE: Build failed with an exception.

* What went wrong:
Could not determine the dependencies of task ':sha512:jsIrTestPackageJson'.
> Could not resolve all dependencies for configuration ':sha512:jsIrTestNpm'.
   > Could not find KHex:extensions:1.1.2.
     Searched in the following locations:
       - https://maven.pkg.github.com/komputing/KHex/KHex/extensions/1.1.2/extensions-1.1.2.pom
       - https://repo.maven.apache.org/maven2/KHex/extensions/1.1.2/extensions-1.1.2.pom
     Required by:
         project :sha512

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 5s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ligi

Oh oops, I forgot to mention this PR relies on this pr: komputing/KHex#14
So this can't be merged until KHex is updated and has new artifacts published

Copy link
Member

Choose a reason for hiding this comment

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

thanks! I merged the KHex PR - but still not sure how this can work afterwards - removing the com.github.komputing would mean it is not resolved via jitpack anymore - but not sure what service would be used to resolve the dependency then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, that was a mistake. I published KHex to mavenLocal and accidentally commited that line

Copy link
Member

Choose a reason for hiding this comment

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

no worries - but that might be a deal breaker for merging - seems jitpack still has problems with KMP: jitpack/jitpack.io#3853

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ligi yeah still only jvm for jitpack afaik. And as I was saying this PR need an new version of KHex published on github artifacts before this can be merged

Copy link
Member

Choose a reason for hiding this comment

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

ah right - we can use github artifacts - then it might not be a blocker - released and thanked you here: https://github.com/komputing/KHex/releases/tag/1.1.3
but this still needs a change here so it consumes the dependency from github artifacts right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it still needs that updated dependency

Copy link
Member

Choose a reason for hiding this comment

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

do you want to update the PR with that?

@BierDav
Copy link
Contributor

BierDav commented May 17, 2023

Any updates on this pr?

@ligi
Copy link
Member

ligi commented May 17, 2023

Any updates on this pr?

yes - see the comments here: #23 (comment)

This was referenced May 17, 2023
Merged
Merged
@BierDav
Copy link
Contributor

BierDav commented May 17, 2023

I have fixed the problems in a new pr: #25

Sorry i originally wanted to make a change request to this pr, but it didn't work so needed to make an additional pull request.

@luca992
Copy link
Contributor Author

luca992 commented May 18, 2023

Updated everything and it uses the new KHex version 👍

@luca992
Copy link
Contributor Author

luca992 commented May 18, 2023

is it.languageSettings.enableLanguageFeature("InlineClasses") actually needed? it causes so many warnings

@ligi ligi merged commit 26f9a6f into komputing:master May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ligi ligi ligi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@luca992 @BierDav @ligi