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

Cross-compile with Scala Native #4279

Closed
wants to merge 1 commit into from

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented Sep 17, 2024

I intend to split this into smaller PR's starting with the macro replacing java reflection. This PR is only meant to showcase the larger picture of the changes and probably will not be merged (although finished and, as of writing, up to date). Nevertheless it would be very helpful to me to see if the CI is able to pass here.

@jchyb jchyb mentioned this pull request Sep 17, 2024
@jchyb jchyb changed the title Cross-compile to with Scala Native Cross-compile with Scala Native Sep 17, 2024
@@ -0,0 +1,3 @@
apt-get install -y clang-12.0
PATH=/usr/lib/llvm-12.0/bin:${PATH}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this is needed? I never had to install clang for native, I think it's included in the images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that's a leftover from the previous iteration, I wonder if I even needed it then? Removing now

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe at some point?

@@ -0,0 +1,12 @@
package org.scalafmt.cli.difflib
Copy link
Contributor

Choose a reason for hiding this comment

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

difflib is now published as munit-diff so you can use that

Copy link
Collaborator

@kitbellew kitbellew left a comment

Choose a reason for hiding this comment

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

this is a huge change, thank you for this. however, i have to admit that i lost track of all the changes halfway through.

here my request: split into separate prs:

  • pure renames (code, tests, etc)
  • refactoring of jvm code which might need to have a custom implementation in native later
  • removing things like difflib, or adding fastparse (still haven't figured out why)
  • adding native at the end

looking at srcLine, I'm afraid there was some manual typing, rather than copy-paste, I'd strongly advise against that, as I can't eyeball everything

runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v2
- uses: olafurpg/setup-scala@v13
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this still used? i thought we replaced with setup-java a while ago...

strategy:
fail-fast: false
matrix:
os: [macOS-latest, ubuntu-latest]
Copy link
Collaborator

Choose a reason for hiding this comment

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

you check for windows-latest below but it's not here...

run: ./bin/scala-native-setup/windows-setup.sh
- if: matrix.os == 'macOS-latest'
name: setup macOS environment
run: ./bin/scala-native-setup/macos-setup.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to implement just one script, say, platform-setup.sh, and in that determine whether we are in Linux, macos or windows, so that we don't need to have this complexity here? after all, someone might want to run stuff from the command line, without GitHub actions

@@ -0,0 +1,3 @@
apt-get install -y clang-12.0
PATH=/usr/lib/llvm-12.0/bin:${PATH}
clang --version
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing newline

@@ -0,0 +1,3 @@
apt-get install -y clang-12.0
PATH=/usr/lib/llvm-12.0/bin:${PATH}
clang --version
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing newline... could you please check other files as well?

@@ -457,7 +450,7 @@ object State {
adjustMargin: Int => Int,
firstLength: Int,
): (Int, Int) = {
val matcher = getStripMarginPattern(pipe).matcher(syntax)
val matcher = getStripMarginPatternWithLineContent(pipe).matcher(syntax)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did you need to add WithLineContent?

@@ -0,0 +1,9 @@
package org.scalafmt.interfaces;
Copy link
Collaborator

Choose a reason for hiding this comment

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

semicolon?

@@ -1,6 +1,6 @@
package org.scalafmt

import org.scalafmt.CompatCollections.ParConverters._
import org.scalafmt.CompatCollections.CompatParConverters._
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why this is needed. we have our own package already, why not define everything there?

@@ -0,0 +1,8 @@
binPack.literalArgumentLists = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a new test?

@@ -314,6 +330,7 @@ trait CliTestBehavior {
}

test(s"scalafmt (no matching files) is okay with --mode diff and --stdin: $label") {
assumeNotDynamicOnNative()
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not create a method which wraps test() and calls this method before the rest of the body?

@kitbellew
Copy link
Collaborator

also, please add macros separately. i don't know why, but I didn't see them at all, they somehow got lost in the sea of changes.

@jchyb jchyb force-pushed the feature/scala-native branch 2 times, most recently from 2a9bd3a to 5d96b36 Compare September 18, 2024 15:34
matrix:
os: [windows-latest, ubuntu-latest]
runs-on: ${{ matrix.os }}
steps:
Copy link
Contributor

Choose a reason for hiding this comment

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

Github is complaining:

[Error](https://github.com/scalameta/scalafmt/actions/runs/10925443463/workflow)
a step cannot have both the `uses` and `run` keys

Co-authored-by: Tomasz Godzik <tgodzik@users.noreply.github.com>
@jchyb
Copy link
Contributor Author

jchyb commented Sep 27, 2024

Merged via related PRs

@jchyb jchyb closed this Sep 27, 2024
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.

3 participants