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

Curl install launcher in your repo #3532

Merged
merged 1 commit into from
Mar 4, 2025
Merged

Conversation

joan38
Copy link
Contributor

@joan38 joan38 commented Mar 3, 2025

I was recording this video today and thought we could shorted further how long it takes for people to start with scala.

Also I think the section Standalone launcher should be before Advanced Installation, what do you think?
Should we also rename scala-cli to scala?

Copy link

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

This doesn't work for all the people who will clone your repo in Windows. You're making it harder for them.

@joan38
Copy link
Contributor Author

joan38 commented Mar 3, 2025

@sjrd I added the Windows "curl" now

@sjrd
Copy link

sjrd commented Mar 3, 2025

I think you misunderstood my comment. The problem is not that the instructions on Windows were missing. It's that if you are on Linux you will commit the executable for Linux, but then users on other systems won't be able to use your repo.

This is bad practice, honestly. System-dependent things should be installed at the system level. Everything else can fit in a repo.

@Gedochao
Copy link
Contributor

Gedochao commented Mar 3, 2025

@sjrd @joan38 While I don't have a strong opinion here, and I'm not sure if it is indeed a good or bad practice, it is most definitely a common one... It's exactly how Mill is recommended to be used (and indeed is in this very repository).

I'd be okay to include this in the doc, regardless.
Perhaps the disputable part is whether we should have the paragraph about recommending this practice, or is mentioning the links sufficient.

@tgodzik @kasiaMarek thoughts?

@sjrd
Copy link

sjrd commented Mar 3, 2025

It's exactly how Mill is recommended to be used (and indeed is in this very repository).

Yes, and it makes Mill builds typically unusable on Windows.

@Gedochao
Copy link
Contributor

Gedochao commented Mar 3, 2025

We do have a mill.bat script committed, as well as mill...
@philwalk any opinion on this?
to be fair, I do not dev on Windows and may be unaware of the pain points, here.

@sjrd
Copy link

sjrd commented Mar 3, 2025

If you actually have mill.bat and you update it, and it's tested in CI, then this build is fine.

I said "typically" because most people don't actually do that.

@joan38 joan38 force-pushed the launcher branch 2 times, most recently from f6a3311 to 8435ef1 Compare March 3, 2025 22:52
@joan38
Copy link
Contributor Author

joan38 commented Mar 3, 2025

This PR only adds the commands for quickly setting up the launcher scripts. What people already do with cumbersome copy/past and chmod +x.

@sjrd's concern is valid, that's why I pushed a version with the commands to also download the script of opposite platforms.

@SethTisue
Copy link
Contributor

SethTisue commented Mar 3, 2025

We recommend committing a scala executable for each OS together with your code [...]

I really don't think we should encourage this. It's a rat's nest — not just different OS, but also different CPU architectures for each OS.

@joan38
Copy link
Contributor Author

joan38 commented Mar 4, 2025

@SethTisue Only OS because the script takes care of the architectures

@SethTisue
Copy link
Contributor

SethTisue commented Mar 4, 2025

Oh, I misunderstood what you meant by "executable", sorry.

(If we are to give this advice, perhaps it would be better to say "launch script".)

@SethTisue
Copy link
Contributor

There was a time when tons of sbt-based projects had an sbt launch script in-repo, but I've always seen this as a relic of the Mark Harrah era when there was no standard launch script. Once sbt-extras died and Eugene committed to shipping a standard launch script, pretty much everybody stopped including it.

The committed launch scripts had a terrible tendency to fall out of date, and Scala Steward doesn't know how to update them.

Though I can see that the issue can be debated one way or the other, I'm firmly in the camp that thinks that committing these scripts is actually bad practice. Build tools (and semi-build-tools like Scala CLI) should be installed at the system level.

If we want to document that some people feel differently, I wouldn't be strongly opposed, but as currently written, this PR presents it as standard practice. I think it is very much not standard practice at all.

@joan38
Copy link
Contributor Author

joan38 commented Mar 4, 2025

as currently written, this PR presents it as standard practice

The sentence was in the Launcher section but I'm removing it since it's controversial.
This PR now only adds the commands to download the scripts.

Copy link
Contributor

@Gedochao Gedochao left a comment

Choose a reason for hiding this comment

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

LGTM

@Gedochao Gedochao merged commit 49c6696 into VirtusLab:main Mar 4, 2025
77 checks passed
@philwalk
Copy link
Contributor

philwalk commented Mar 4, 2025

We do have a mill.bat script committed, as well as mill

I agree with @sjrd w.r.t. problems on Windows. For anyone working from git-bash or other bash shell environments, a .bat file is not adequate. The following wrapper script (if placed in the bin directory with the .bat file) mostly solves the problem for me.

#!/bin/bash
exec "$0.bat" "$@"

@joan38 joan38 deleted the launcher branch March 4, 2025 16:20
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.

5 participants