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

Bloop-rifle update #351

Merged
merged 9 commits into from
Sep 22, 2023
Merged

Conversation

SlowBrainDude
Copy link
Contributor

Upstream moved, and some follow-up changes need
to be included as well.

Graal Native Image also needed some adjustments.

Important:
This PR depends on scala-cli/bloop-core#153.

I'm not entirely sure about the build changes. But there seem to be some dependencies missing for 2.12. Should support for Scala 2.12 be dropped maybe altogether?

@oyvindberg
Copy link
Owner

Fantastic, thanks!

I'm busy today and tomorrow - will look at it first thing after that! :)

@SlowBrainDude
Copy link
Contributor Author

SlowBrainDude commented Sep 6, 2023

The whole point is actually getting a new version of bloop-rifle in, which includes a bugfix for an issues I've run into.

I've fixed it locally and I'm running a self built bleep version, which works fine so far. So I'm quite confident the update won't make anything worse. Test still pass…

Bleep as such is very cool so far! Thanks for making it! 👍

But I guess it still needs some love here and there. Let's see, maybe I come back with more later on. 😄

@oyvindberg
Copy link
Owner

But there seem to be some dependencies missing for 2.12. Should support for Scala 2.12 be dropped maybe altogether?

I'm ok with dropping 2.12 completely at this point.

For context: The reason the 2.12 support is there is ironically future-proofing. By having bleep cross-compiled to 2.12, it's possible to port sbt plugins without upgrading the scala version they are written in, and then use them in bleep scripts written in 2.12. I'm willing to drop this, since porting random 2.12 code to 2.13 is normally easy. Dropping 2.13 would not be acceptable yet, it's only recently that it was possible to port the sonatype integration code to 3.

Also I thought it beneficial in case it was needed to embed bleep in sbt or vice versa, but I don't see a path in that direction for now.

@oyvindberg
Copy link
Owner

Ok, so AFAIU this PR really depends on an unmerged PR in bloop-core.

If the build is green I'll go ahead and merge this soon, so the next PR will be a simple version bump

@oyvindberg
Copy link
Owner

Ok, not green. I'll debug it from here 👍

@SlowBrainDude
Copy link
Contributor Author

Hi! Thanks for having a look.

I can update the PR as soon as the upstream change is in.

The upstream change is the solve reasons for this here… 😄

In case there will be also a bloop-rifle update for 2.12 there is no need to drop the 2.12 support here.

Regarding the Native Image related stuff: Like said in the comments I'm not sure why this popped up actually. There is maybe some issue with double dependencies in different version. But it could be also that my local build got broken at some point, and wasn't really clean.

So no hurry, we need to wait for upstream anyway. Than I will have a second look.

@oyvindberg
Copy link
Owner

I did some work on this, and it seems to be working well.

I'll merge this first version and a small pipeline of related changed over the next days. then we'll upgrade to the version you want in a subsequent PR 👍

@SlowBrainDude
Copy link
Contributor Author

SlowBrainDude commented Sep 16, 2023

Thanks for looking into that!

You found the issues with broken compile when not running over BSP? I've just wanted to dig into that.

Could you push whatever you have so I could build already a local version that works?

I have not noticed the breakage at the time I've submitted the PR as I was just running bleep as BSP server. Which worked fine. But the command line compile was broken as bloop put things in different folders than before…


Please note that the version of the dependency I want still doesn't exist. We need to wait for upstream…

I've opened additionally a ticket in the scala-cli repo, as the bug in bloop-rifle affects also them. Maybe they will be now willing to do something about it.

I understand that a lot of people don't care to have million runtimes in billion versions installed; but imho this is a Windows / MacOS style security catastrophe. All the runtimes, and actually any other stuff installed behind the back of the package manager, are ticking time bombs, just waiting to be exploited in some attack, as this stuff never gets security updates. (And I don't even talk about all the issues with the external downloaders, like that they don't support any package signatures, which opens the floodgates to "supply-chain attacks" by simple DNS manipulation.)

So I think it's important that things work fine with a system provided runtime properly installed through package management.

@oyvindberg
Copy link
Owner

Thanks for looking into that!

You found the issues with broken compile when not running over BSP? I've just wanted to dig into that.

I found a bunch of smaller things, but not this in particular. it does seem to work however.

Could you push whatever you have so I could build already a local version that works?

Absolutely. https://github.com/oyvindberg/bleep/compare/bloop-rifle-update-2?expand=1

If you're ok with it I'll just push some of the commits which are direct fallout from this to your branch. then create a followup PR or two and merge everything. I'm busy sunday, but monday I have time for this.

I understand that a lot of people don't care to have million runtimes in billion versions installed; but imho this is a Windows / MacOS style security catastrophe. All the runtimes, and actually any other stuff installed behind the back of the package manager, are ticking time bombs, just waiting to be exploited in some attack, as this stuff never gets security updates. (And I don't even talk about all the issues with the external downloaders, like that they don't support any package signatures, which opens the floodgates to "supply-chain attacks" by simple DNS manipulation.)

So I think it's important that things work fine with a system provided runtime properly installed through package management.

There is also some friction between security on one side and repeatable builds/consistent setup on the other side. Personally I focus more on the latter. That said though, I really want to invest in tooling a la bleep build update-deps which also considers bleep version, JVM version and so on. finally integrate it with scala steward or similar so it's easy to stay up to date.

@SlowBrainDude
Copy link
Contributor Author

If you're ok with it I'll just push some of the commits which are direct fallout from this to your branch. then create a followup PR or two and merge everything.

This sounds great!

I'm busy sunday, but monday I have time for this.

Whenever you have time.

Thanks for looking into this! Great to see such quickly responding OpenSource project.

There is also some friction between security on one side and repeatable builds/consistent setup on the other side.

I don't think this has much to do with reproducible builds. Actually it's even counter the idea of reproducible builds, imho:

The point of reproducible builds is to be able to build stuff independently, likely even with other toolchains, and arrive at the very same result.

Forcing your toolchain on everybody is more like the Docker approach:

docker-works-on-my-machine

Exactly this is the reason why there is more then enough JVM stuff that works only on "Oracle Java 1.5"… 😉

Of course I know that there are people who like to follow the Windows / MacOS approach because "it's convenient". Likely the same people who don't understand why Windows / MacOS is full of malware, whereas Linux has no issues, even it's technically the same trash as everywhere. (I may say that, I'm using Linux for over two decades so I know that they don't do magic; it's just the much more sane approach to dependencies and software management in general that keeps things reasonable safe. Of course there are people who try to break even that; bullshit like Flatpak exists…)

I really want to invest in tooling a la bleep build update-deps which also considers bleep version, JVM version and so on. finally integrate it with scala steward or similar so it's easy to stay up to date.

Before people jump to building their own package managers, it would be good to first make it possible to actually package Scala stuff. It has reasons why there are no official distri Scala package anywhere, and you can't apt install anything containing Scala code…

A build tool that doesn't "download the internet", but works finally in some sane way, would be one of the puzzle pieces to fix that.

Of course I'm not going to hold you back building the next package manager that installs stuff behind the back of the system. But we have already more then enough of such, imho.

Anyway, having Scala be properly integrated into the OpenSource world is frankly currently out of scope, as it's impossible to build the Scala compiler from source. So there is currently no alternative to execute random binaries downloaded form the internet and hope the best…

But at least I don't want to double-down on that. What can be provided by trusted sources (like JVMs built by Debian) should come from there.

This doesn't mean I don't like the current project dependency update feature. It's much better than the one of the competition. The approach to have a tool re-writable config is very good! Smart design decision, which I applaud.

@SlowBrainDude
Copy link
Contributor Author

SlowBrainDude commented Sep 18, 2023

https://github.com/oyvindberg/bleep/compare/bloop-rifle-update-2?expand=1

Thanks for sharing!

Frankly I've encountered the same bug as with my previous local builds.

bleep compile behaves differently, resulting in bleep publish-local to emit .jars that don't contain any class files.

The issue is that something in the directory structure of bloop changed.

In the folder $project/.bleep/builds/normal/.bloop/$module/ the original bleep (version 0.0.2) creates

  • classes/
  • bloop-bsp-clients-classes/

and fills both folders with compilation results. (In the later under an additional sub-folder like classes-a4584sSJQYqfYDlYJ62I7w==)

But the new version built from the above branch creates

  • bloop-bsp-clients-classes
  • bloop-internal-classes

and the former folder stays empty.

I guess that's a changed behavior in bloop-rifle as I couldn't find any sign of the above path fragments anywhere in the bleep code.

I don't really know how to debug that as I don't know where exactly bleep configures bloop

@oyvindberg
Copy link
Owner

Interesting! I did a bleep dist on my machine, and the jar files did have classes inside.

Have you tried to explicitly shut down the compile server and tried again? Possibly a mismatch somewhere.

It's by the way a well-known thing with bloop this behavior. By default it puts your classes in weird directories unless you declare that you "own" the build, in which case class files should end up in classes.

I'll try to look a bit further tonight or tomorrow, would be nice to ship that branch and being back to current

@SlowBrainDude
Copy link
Contributor Author

Have you tried to explicitly shut down the compile server and tried again? Possibly a mismatch somewhere.

I'll did all that.

When testing I'm restarting the compile server anyway. (Otherwise I wouldn't even run into any issues with bloop-rifle, as this "works fine" as long an old instance is running.)

I also clean cache directories while testing…

This one here appears even with a complete clean build where everything gets downloaded fresh.

As I didn't mention that until now: I'm testing the bleep Graal Native Image (built with bleep native-image).

At the time I've submitted this PR I also didn't know about that issues. The BSP server works fine. Also one could build the bleep native image without issues. Just that the result wasn't any more able to publish-local working stuff. (There is no error or so, just that the resulting .jars contain just a Manifest. No class files…)

Do you know actually why bloop compiles things at least twice? That looks wasteful. Also you have this way a problem keeping stuff in sync… I'm not sure what they're doing here. (But likely it has reasons. I just don't see them.)

@oyvindberg
Copy link
Owner

Indeed @SlowBrainDude . I could reproduce it now. It was I who forgot to clean :D

Thanks for noticing, I would definitely have shipped this

@SlowBrainDude
Copy link
Contributor Author

Thanks for having a second look at it. 👍

At least upstream is now ready: https://github.com/scala-cli/bloop-core/releases

In case I can help here somehow just tell. But like said above, I have no clue at the moment where to look.

In case I have time for hat I would try to bisect the upstream lib. Maybe the behavior change happened after the package rename. Otherwise it's going to be a little bit complicated to look for the change.

Of course, In case you know where to look it could be also quite simple to fix that, without looking for the upstream cause. I hope it's like that.

@oyvindberg
Copy link
Owner

I think bisection is the simplest option for now actually. I dug into upstream last night, but couldn't spot anything which was wrong.

The logic controlling this lives here

data.add("ownsBuildFiles", new JsonPrimitive(true))

You're supposed to set that the build tool owns the build, and the class files should then go to the correct place.

Another place to look is the scala-cli changes, as it's very likely the bug has appeared there as well

I'll find some time for this, but it'll be a bit on and off, so if you find something that's helpful

SlowBrainDude and others added 5 commits September 22, 2023 22:21
Upstream moved, and some follow-up changes need
to be included as well.

Graal Native Image also needed some adjustments.
Context:
The reason the 2.12 support is there is ironically future-proofing. By having bleep cross-compiled to 2.12, it's possible to port sbt plugins without upgrading the scala version they are written in, and then use them in bleep scripts written in 2.12. I'm willing to drop this, since porting random 2.12 code to 2.13 is normally easy. Dropping 2.13 would not be acceptable yet, it's only recently that it was possible to port the sonatype integration code to 3.

Also I thought it beneficial in case it was needed to embed bleep in sbt or vice versa, but I don't see a path in that direction for now.
@oyvindberg
Copy link
Owner

ok, got it! the native-image configuration lost the entry for BloopExtraBuildParams.

Bumped the bloop version as well, so this is ready to go now 🎈

@SlowBrainDude
Copy link
Contributor Author

OH NO! It was me… 💩

Sorry for that trouble.

That's why you should not touch things even everything looks like it "works fine" without them…

@SlowBrainDude
Copy link
Contributor Author

Me idiot forgot to place a comment there to ask what this actually is, and why it compiles without it.

Now I remember I wanted to do that. But just forgot. Really sorry! 😞

@oyvindberg
Copy link
Owner

oh don't worry, stuff like that happens.

so yeah, those files instruct graalvm native image to generate code to support reflection for some types. and the whole bsp4j thingie we inherit from bloop-rifle is ultimately based on that. ugh.

@SlowBrainDude
Copy link
Contributor Author

SlowBrainDude commented Sep 22, 2023

oh don't worry, stuff like that happens.

It should not. This was really stupid. When I'm touching code I'm not sure about I should at least ask.

But in this case I've overlooked this place in the end myself… 😞

those files instruct graalvm native image to generate code to support reflection for some types

I've got that. The native image crashed and was complaining about not marked reflective access as I've first tried. That's was the reason I've touched stuff there in the first place. But I touched too much…

Strange that the GraalVM didn't crash or at least complain about these classes here.

But I guess I just don't know enough about Native Image currently. Maybe there are some switches to let it search for such cases as here?

@oyvindberg
Copy link
Owner

But I guess I just don't know enough about Native Image currently. Maybe there are some switches to let it search for such cases as here?

yes, there is an agent you can give to the JVM so that it'll record all reflections and so it encounters. I haven't really needed it that much, so I haven't looked into it.

@SlowBrainDude
Copy link
Contributor Author

there is an agent you can give to the JVM so that it'll record all reflections and so it encounters

I've heard about that. But that's something you do before you build your native images.

But I was more thinking about what happened here. The the native image didn't crash or complain. It just silently failed. (For the other reflective calls it just crashed, with a helpful error message.)

@oyvindberg
Copy link
Owner

But I was more thinking about what happened here. The the native image didn't crash or complain. It just silently failed. (For the other reflective calls it just crashed, with a helpful error message.)

yeah, that would have been nice. I guess it is so common with reflection in java that crashing is likely a bad choice. complaining would have been 👍

This has come up before, that when building bleep while it changes bloop release in the middle is complicated.

try to first compile and build a dev-script with the old bloop, turn it off, and then run the devscript version
@oyvindberg oyvindberg merged commit a7ab124 into oyvindberg:master Sep 22, 2023
5 checks passed
@oyvindberg
Copy link
Owner

🥳

@SlowBrainDude
Copy link
Contributor Author

SlowBrainDude commented Sep 22, 2023

Does the last commit need some external documentation?

I also had trouble building it locally. Now just trying what I've read in the CI config.

But in case this is expected to happen again in the future when updating bloop, maybe this magic incantation ("turning it off and on again" 😁) should become a documented build step?

@SlowBrainDude
Copy link
Contributor Author

Thank you so much for pushing it over the finish line! ❤️

@oyvindberg
Copy link
Owner

oyvindberg commented Sep 23, 2023

But in case this is expected to happen again in the future when updating bloop, maybe this magic incantation ("turning it off and on again" 😁) should become a documented build step?

So i think this only applies when upgrading bloop. There is some functionality within bloop-rifle to check bloop version when it's connecting, and if bloop is too old it shuts it down (check usage of BloopRifleConfig.BloopVersionConstraint). Maybe bleep is doing something wrong, because by the interface of bloop-rifle I'd expect it to both shut it down and start a new one, but something there is off.

I don't intend to dig in and fix this anytime soon (it's the magical combination of difficult and boring). If anything, we could try to patch the error message to something which asks you to turn it off and on again.

Thank you so much for pushing it over the finish line! ❤️

And thanks for the contribution! More people is really needed for bleep to reach it's potential :)

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.

2 participants