Give some love to the project #70

guizmaii · 2023-07-16T13:18:21Z

Remove sbt 1.0.0 support
Update Scala version
Update sbt and sbt plugins
Remove deprecated sbt syntax usages
Update Scalafix and improve its configuration
Install Scala Steward to help us maintain this project
Fix Scalafix issues in code

The tests are failing, but as they're also failing on the main branch, I'm unsure if I broke something.
I tried to fixed the tests but I don't understand why the agent-test tests are failing

Cc @ckipp01 @olafurpg

build.sbt

- Remove sbt 1.0.0 support - Update Scala version - Update sbt and sbt plugins - Remove deprecated sbt syntax usages - Update Scalafix and improve its configuration - Install Scala Steward to help us maintain this project - Fix Scalafix issues in code

ckipp01

Thanks a lot for taking a look at this @guizmaii. As you pointed out, this repo could use some love, but doesn't really have anyone steering the ship on it anymore. I know for myself I don't use it at all, so there isn't a lot of incentive for me to look at it regularly. If this is something you use, would you be interested in joining the group of maintainers for it? I'll happily give you access.

So I just did an initial run through and left a couple nits. I need to look closer at what is going on with the test failure because locally on mac I'm getting failures where before I don't remember getting them. I'm also on an M1 macbook, which won't even load the old build on main. Let me take a closer look and I'll comment back what I find and we can try to get this across the finish line. Again, thanks for taking a look at this.

O, and regarding dropping the 1.3.x support. I think that's fine, especially if it makes maintenance easier at all. I know a lot of the sbt plugin under the sbt org have been doing the same, so I think it's fine to follow here.

ckipp01 · 2023-07-17T11:50:35Z

.github/workflows/scala-steward.yml

Let's remove this for now. We actually have our own scalameta steward here which we can add this repo to once we get this merged in.

If possible, I'd prefer to keep it as, usually, external Scala Steward instances are making PRs from a fork, which makes it harder (if not impossible) to commit on the branches made by Scala Steward (if we need to fix something on one of the update branches)

It should be fine to switch this. We do the same for all the rest of the scalameta projects. Since the PRs are not on the main branch of the fork, a maintainer of the repo is able to add another commit to it.

ckipp01 · 2023-07-17T11:54:05Z

.scalafix.conf

+RemoveUnused {
+  imports = false // See https://github.com/scalacenter/scalafix/blob/v0.11.0/docs/rules/OrganizeImports.md#configuration
+}


Super nit, but we are sort of mixing styles with how we have this setting defined and how we have the ones below defined. Can we change this to

Suggested change

RemoveUnused {

imports = false // See https://github.com/scalacenter/scalafix/blob/v0.11.0/docs/rules/OrganizeImports.md#configuration

}

RemoveUnused.imports = false

Just to be consistent with how we do the DisableSyntax down below. We can do the same with Disable.ifSynthetic

ckipp01 · 2023-07-17T11:57:34Z

build.sbt

@@ -42,7 +43,6 @@ lazy val plugin = project
  .settings(
    moduleName := "sbt-native-image",
    sbtPlugin := true,
-    sbtVersion.in(pluginCrossBuild) := "1.0.0",
    crossScalaVersions := List(scala212),


Is this still needed at all?

guizmaii · 2023-07-17T12:08:24Z

Hey @ckipp01,

Thanks for your comment and initial review :)

If this is something you use, would you be interested in joining the group of maintainers for it?

I'm already maintaining/helping to maintain too many things but I'll use this plugin, so yes I'm interested in joining 🙂

This also bumps some stuff in the CI, even though we'll rip it out after.

ckipp01 · 2023-07-18T09:56:33Z

Ah annoying, so I tried to bump to 22.3.0 since I was having issues locally with that, but then I also see that the way it's installed in CI now is with Jabba, which we should just ditch and do what you did with the setup-graal. I was also trying to get things locally to pass on my machine, but things fails since I don't have the NATIVE_IMAGE_COMMAND set locally. I don't actually like that you have to set that manually. So here is what I propose.

I might actually take the CI changes PR that you made and put that on top of this just to ensure we can use a newer Graal version in CI. I might also change up the tests to not fail if NATIVE_IMAGE_COMMAND isn't found because locally they still work fine with out it. This will hopefully get us a bit further. I'm honestly fine with merging in if everything isn't 🟢 but I'd still like to get the ubuntu stuff working. I figure we can slowly just keep tackling small things.

Also I hope it's ok that I'm just adding commits on top of this.

We'll use the org one instead

guizmaii · 2023-07-19T13:56:51Z

Also I hope it's ok that I'm just adding commits on top of this

It's completely fine. Thanks for your help, Chris! ❤️

I'm honestly fine with merging in if everything isn't 🟢 but I'd still like to get the ubuntu stuff working. I figure we can slowly just keep tackling small things.

Agreed. I'd like to see everything green but we need to make small steps here.

I might actually take the CI changes PR that you made and put that on top of this just to ensure we can use a newer Graal version in CI. I might also change up the tests to not fail if NATIVE_IMAGE_COMMAND isn't found because locally they still work fine with out it. This will hopefully get us a bit further.

Go for it 🙂
Tell me if you need help. I'm always available on Discord if you have questions.

ckipp01 · 2023-07-27T16:27:06Z

I haven't forgotten about this @guizmaii, just been swamped with other stuff. I will get back to it soon.

guizmaii force-pushed the support_new_graalvm_builds branch from 5880888 to e8fac8a Compare July 16, 2023 13:38

guizmaii changed the title ~~Give some love to the project ❤️~~ Give some love to the project Jul 16, 2023

guizmaii force-pushed the support_new_graalvm_builds branch 2 times, most recently from 31da365 to 9f3d5db Compare July 16, 2023 13:46

guizmaii commented Jul 16, 2023

View reviewed changes

build.sbt Show resolved Hide resolved

guizmaii mentioned this pull request Jul 16, 2023

Update Scalafmt config #72

Open

guizmaii force-pushed the support_new_graalvm_builds branch 2 times, most recently from 8436a51 to f76d299 Compare July 16, 2023 13:57

Give some love to the project

f233a78

- Remove sbt 1.0.0 support - Update Scala version - Update sbt and sbt plugins - Remove deprecated sbt syntax usages - Update Scalafix and improve its configuration - Install Scala Steward to help us maintain this project - Fix Scalafix issues in code

guizmaii force-pushed the support_new_graalvm_builds branch from 0581ad3 to f233a78 Compare July 16, 2023 14:07

try to avoid CI failures

a177ab5

guizmaii force-pushed the support_new_graalvm_builds branch from 2422fdb to d5b3a63 Compare July 16, 2023 14:24

Update example projects

1d5f26b

guizmaii force-pushed the support_new_graalvm_builds branch from d5b3a63 to 1d5f26b Compare July 16, 2023 14:32

clean

4ba6939

guizmaii marked this pull request as draft July 16, 2023 15:35

guizmaii force-pushed the support_new_graalvm_builds branch from 2877c87 to 31b34e0 Compare July 16, 2023 15:52

Try fix scripted tests

f8cee7a

guizmaii force-pushed the support_new_graalvm_builds branch from 31b34e0 to f8cee7a Compare July 16, 2023 15:53

guizmaii marked this pull request as ready for review July 16, 2023 15:55

ckipp01 reviewed Jul 17, 2023

View reviewed changes

deps: bump default nativeImageVersion to 22.3.0

7f29c8e

This also bumps some stuff in the CI, even though we'll rip it out after.

ckipp01 force-pushed the support_new_graalvm_builds branch from 46c4025 to 7f29c8e Compare July 18, 2023 09:46

ckipp01 added 3 commits July 19, 2023 11:11

refactor: update tests to use 2.13.11

8a0cb20

refactor: remove local scala-steward config

e8bfe1d

We'll use the org one instead

refactor: start using setup-graal

101b85e

ckipp01 force-pushed the support_new_graalvm_builds branch from ec7c920 to 101b85e Compare July 19, 2023 09:43

fix: get rid of stray folder screwing up test

b8012a5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give some love to the project #70

Give some love to the project #70

guizmaii commented Jul 16, 2023 •

edited

Loading

ckipp01 left a comment •

edited

Loading

ckipp01 Jul 17, 2023

guizmaii Jul 17, 2023 •

edited

Loading

ckipp01 Jul 17, 2023

ckipp01 Jul 17, 2023

ckipp01 Jul 17, 2023

guizmaii commented Jul 17, 2023

ckipp01 commented Jul 18, 2023 •

edited

Loading

guizmaii commented Jul 19, 2023

ckipp01 commented Jul 27, 2023

Give some love to the project #70

Are you sure you want to change the base?

Give some love to the project #70

Conversation

guizmaii commented Jul 16, 2023 • edited Loading

ckipp01 left a comment • edited Loading

Choose a reason for hiding this comment

ckipp01 Jul 17, 2023

Choose a reason for hiding this comment

guizmaii Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

ckipp01 Jul 17, 2023

Choose a reason for hiding this comment

ckipp01 Jul 17, 2023

Choose a reason for hiding this comment

ckipp01 Jul 17, 2023

Choose a reason for hiding this comment

guizmaii commented Jul 17, 2023

ckipp01 commented Jul 18, 2023 • edited Loading

guizmaii commented Jul 19, 2023

ckipp01 commented Jul 27, 2023

guizmaii commented Jul 16, 2023 •

edited

Loading

ckipp01 left a comment •

edited

Loading

guizmaii Jul 17, 2023 •

edited

Loading

ckipp01 commented Jul 18, 2023 •

edited

Loading