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.

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

Rework source sets to have rich hierarchy #130

Merged
merged 7 commits into from Nov 7, 2022
Merged

Rework source sets to have rich hierarchy #130

merged 7 commits into from Nov 7, 2022

Conversation

C2H6O
Copy link
Contributor

@C2H6O C2H6O commented Nov 7, 2022

Fixes or Changes Proposed

  • My change (#issue-number-if-applicable)

According to https://youtrack.jetbrains.com/issue/KT-54512 ticket, this repo only had a commonMain source set, while other source sets were emulated by customizing source set locations. This PR reworks source sets so that they form a parent-child hierarchy, which addresses #129.

PR Checklist

  • I have added a CHANGELOG.md entry for any noteable changes or fixes.
  • I have added test coverage for any new behavior or bug fixes.

@C2H6O
Copy link
Contributor Author

C2H6O commented Nov 7, 2022

@benasher44 with these changes, I can successfully publish both (playground and pgf) projects. It was just a matter of taking the emulated locations and mapping them into a hierarchy

Copy link
Owner

@benasher44 benasher44 left a comment

Choose a reason for hiding this comment

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

ah nice!

@benasher44
Copy link
Owner

thanks for doing this!

Comment on lines +72 to +73
val jsMain by getting { dependsOn(nonJvmMain) }
val jsTest by getting { dependsOn(nonJvmTest) }
Copy link
Owner

Choose a reason for hiding this comment

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

looks like jsMain doesn't exist. guessing it's named differently between the IR and non-IR JS variants

Copy link
Owner

Choose a reason for hiding this comment

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

i would comment this out, list the tasks, see what the names are like, and update based on that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

weird, I'm not really sure how it was compiling previously, because js and jvm targets were only declared if the host was a Mac machine. I added a bunch of HostManager checks and it looks like it is building successfully now

Copy link
Owner

@benasher44 benasher44 left a comment

Choose a reason for hiding this comment

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

looks good! any other edits?

@C2H6O
Copy link
Contributor Author

C2H6O commented Nov 7, 2022

Nope! I'll merge 'er in

@C2H6O
Copy link
Contributor Author

C2H6O commented Nov 7, 2022

oh nvm, I can't

@benasher44 benasher44 merged commit e58d383 into benasher44:master Nov 7, 2022
@@ -5,6 +5,10 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/), and this
project adheres to [Semantic Versioning](https://semver.org/).

## [0.6.0] - 2022-11-07
Copy link
Owner

@benasher44 benasher44 Nov 7, 2022

Choose a reason for hiding this comment

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

lol (the date)

Copy link
Owner

Choose a reason for hiding this comment

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

will try to release today, but no promises!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about the date? looks correct

Copy link
Owner

Choose a reason for hiding this comment

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

Oh it looks fine. It's just optimistic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, heh. Not a rush at all!

I thought I messed up the month/day order, but you've been using a sane system since the beginning :)

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.

None yet

2 participants