Add contributing quickstart guide #3496

tanishiking · 2023-09-21T12:54:47Z

This PR adds a quickstart page for contributing to ScalaNative + fixed the outdated commands.
This page is intended to provide the minimum information necessary to get started working on the ScalaNative repository. Any further information can be described on other pages and linked to from the quickstart guide.

Any comments from both experienced ScalaNative developer and newbie to ScalaNative 👍

FYI @kyouko-taiga

kyouko-taiga · 2023-09-21T13:04:33Z

docs/contrib/quickstart.rst

+
+- ``testsJVM3/test`` - run ``tests3/test`` on JVM
+- ``testsExtJVM3/test`` - run ``testsExt3/test`` on JVM
+- ``test-all`` - run all tests, ideally after ``reload`` and ``clean``


One thing I have noticed is that reloading will reset the version of Scala you might have configured when you started sbt.

% sbt "++3.3.0; shell" ... sbt:scala-native> scalaVersion [info] 3.3.0 sbt:scala-native> clean; reload ... sbt:scala-native> scalaVersion [info] 2.12.18

I might be doing something wrong but that definitely was a surprise for me. Maybe it deserves a mention in this guide.

I think it's more like sbt usage?

You don't need to decide the Scala version to use when you launch sbt shell

You can just run $ sbt to launch sbt shell, no need for ++x.y.z and shell

You can change the scala version in sbt shell whenever you want

$ sbt ... sbt:scala-native> scalaVersion [info] 2.12.18 sbt:scala-native> ++3.3.0 ... sbt:scala-native> scalaVersion [info] 3.3.0

I'm not sure we should add explanation for that here.
And do we want to document test-all, since it takes so long and I personally never used in my local development.

I think it's more like sbt usage?

OK, I understand.

I think I would still add something like "remember that reload will cause the session to be restarted fresh, meaning that your configurations (e.g., ++3.3.0) will be reset as well."

And thanks for the explanations, by the way ;)

And do we want to document test-all, since it takes so long and I personally never used in my local development.

I think we should. FWIW, the first thing I always try to do when I get started with an open source project is to try running the most comprehensive test suite possible.

I think it is better to point to something a little less comprehensive to start like tests3/test or the tools test. The testsJVM will run tests against the VM so the is good to mention for javalib. You might want to mention the 2_13 and 2_12 versions as well. We can't assume we have sbt experts.

I hear you but my two cents is that I still believe that there should be a way for someone cloning SN for the first time to make sure everything works as expected, even if test-all is a command that's almost never run in the "standard development workflow".

You are correct but this is very much a WIP and we may have failures especially with new versions of software or different platforms - what we are trying to do is hard and 0.5.0 has not really settled down 100%.

OK, I understand.

If we can agree that the goal is to eventually be able to run test-all on supported platforms, why not keep a brief record of this discussion in the docs for the next me? I would suggest something like this:

Once you've installed all dependencies, run the `test-all` command in `sbt` to confirm that your environment is properly configured. It will run every test defined in this project, which might take a while. > Note: it is possible that some tests ran by `test-all` might not pass or compile on your system. We are currently working on stabilizing this test suite for our supported platforms. As of this writing (<insert date>), we expect all tests to pass on Linux <insert distribution> X1.Y1 with LLVM X2.Y2, `sbt` X3.Y3, <insert other known dependencies>. > > Should `test-all` fail on your machine, you may try to run smaller test suites one after the other, specifically `test-runtime`, `test-tools`, <insert other test commands>. > Known issues are <insert known issues>.

I just added a disclaimer that test-all may fail 1e57fea
I hope someone who know more about test-all can update the sentence later.

kyouko-taiga · 2023-09-21T13:06:33Z

docs/contrib/quickstart.rst

+
+It's convenient to run the ``sandbox`` project to verify the build works as expected.
+
+Publish Locally


Publishing is a foreign concept to me as an outsider coming with a C++ background. Would it make sense to add a sentence describing what that means or a link to an already existing definition?

As you may noticed "publish" means releasing, and publish locally means release ScalaNative only for local use.
I use this phrase because sbt does https://www.scala-sbt.org/1.x/docs/Publishing.html

Do you think "Release Locally" makes more sense? Or do you think pasting a link to https://www.scala-sbt.org/1.x/docs/Publishing.html helps?

Thanks for these explanations.

"Release locally" might make more sense for a C++ developer but if the vocabulary is already well established in the sbt world then a link to the existing definitions is probably more appropriate.

ekrich · 2023-09-21T16:27:48Z

This is a good guide to use as a reference - https://github.com/scala-js/scala-js/blob/main/DEVELOPING.md

ekrich · 2023-09-22T00:34:05Z

If it is not in the guide, this may be helpful to run individual scripted. The first to get more verbose output.

sbt:scala-native> set ThisBuild / scriptedBufferLog := false
sbt:scala-native> sbtScalaNative/scripted run/build-library-static

kyouko-taiga · 2023-09-22T09:10:41Z

If it is not in the guide, this may be helpful to run individual scripted. The first to get more verbose output.

Unless I'm missing something it seems like @LeeTibbert suggested that we make sbtScalaNative/scripted harder to find because it wasn't intended to be used directly.

ekrich · 2023-09-22T14:29:47Z

It is in there so I think just adding the setting to get the output is good - https://scala-native.org/en/latest/contrib/build.html

Developers need to know everything but I think that to get started sandbox2_13/run is best along with tests2_13/test. Using version 3 means that a bunch of 2.13 stuff has to get compiled plus the 3 stuff so it is a longer path. testsJVM is good for working on javalib

LeeTibbert · 2023-09-22T17:10:01Z

I am flatout trying to fix the underlying bugs, so what we document makes sense.
Sorry to be short. I hope to return once I have test-scripted sorted.

The absolute top of the document should have at least a date, immediately
after the title. I think Sphinx has a way of automatically generating that.

It might be too much effort to carry along & update a document version.
Giving the Scala Version "This is known to work with the Scala Native version current at the time of writing
(Scala Native 0.5.0-SNAPSHOT). Scala Native is under active development. Your mileage/kilometreage might vary".

The first thing a potential Contributor should see/experience is a welcome, with
some indications that they can ask questions (describe where) for issues where
they have spent more than economical time trying to figure it out.

I support the "tiered" approach. Give the first time reader success early on.

Then there should probably be series of higher or more difficult levels, working up
to the aspirational goal of test-all working. Second step should probably be either
an "write a sandbox test, then write, compile, run (test) your own .scala" Probably
using modification of Test.scala.

A next step would be to run test-tools.

Later step should probably be test-runtime. This would give a first approximate verification
that the existing environment worked (at least for test-runtime)

An entirely separate and more difficult step would be solo test-scripted.
I believe that test-all (or possibly earlier) builds the .rst documentation files (or perhaps
Scala's own doc). That means that the setup for that should be described (and foreshadowed, not
detailed in the introductory "fast success" setup.

Then, there should be a series of 7 or 70 Seals, quite similar to those encountered by Howard Carter a
century ago in 1925.

That is, advanced details (which by nature are subject to change/churn) should be in separate sections
each marked "Advanced" or "Abandon all hope, ye who enter here".

sbtScalaNative/scripted in particular is one of those. I believe there is an effort to phase out the use
of scripted; if so, that section should say that new code should use junit if feasible and that the section
is for historical use and to aid understanding of existing scripted code. I believe it would also be
useful for it to have a direct link to the sbt scripted documentation. That page is hard for a non-expert
scripted or sbt person to find. I know from repeated experience & pain.

Sorry to brain-dump & run. More when I can.

LeeTibbert · 2023-09-22T17:14:33Z

When I first went to University, there was an "Index of Prohibited Books" (well, the title was in Latin).
Books on that list were in a locked cabinet in the cellar of the library. Scholars could ask that the
books be fetched individually, but they had to supply a good reason. Students could walk past, but
they needed to avert their eyes and could not study the titles on the leather bindings.

I am reminded of that experience when I think of sbtScalaNative/scripted and where it should be
documented.

WojciechMazur

Thank you for updating the guides, I have only few additional comments, otherwise looks good to me

WojciechMazur · 2023-09-24T23:07:05Z

docs/contrib/quickstart.rst

+
+- Java 8 or newer
+- sbt 1.5.8 or newer
+- LLVM/Clang 6.0 or newer


We can mention recomended version which I belive would be 15+ (becouse it's using opaque points so it's easier to inspect LLVM IR files when contribution.
When it comes to sbt version doesn't really matter becouse version pinned in project/build.properties would be downloaded and used anyway.

I propose we adopt wording such as:
"
Java 8 is the design center, some newer but not all, or even most, newer methods may be implemented.

For sbt and LLVM/Clang, this is what is used in Scala Native Continuous Integration and is known to work. Other
versions may work, but are not guaranteed. The further an alternate version is from the
known working version, either older or newer, the less likely they are to work".

If we change the LLVM/Clang version here, which I heartily support, we should probably change
the "minimum clang check" hidden somewhere in the codebase, perhaps to a less strict "llvm-13" or
"llvm-11".

When I am not getting kicked by the other legs of the 16 legged donkey, I am chasing one or more
JDK > 17 bugs where the initial variant of sbt may matter, and need to be greater than '1.6.0' or '1.6.2'.

I am also chasing some other cases where I suspect the initial sbt version is important. I need to repeat
and capture what I think I saw. More as I have proof.

WojciechMazur · 2023-09-24T23:23:20Z

docs/contrib/quickstart.rst

+- ``nir3/test`` - run the unit tests for NIR
+- ``tools3/test`` - run the unit tests of the tools: ScalaNative backend


In case of nir running tests in native backend it's fine, but in case of tools the tests would now be skipped. It think it would be better to use nirJVM3 and toolsJVM3 here as this are the dominant targets

Right, moved to JVM targets b17677f

WojciechMazur · 2023-09-24T23:26:15Z

docs/contrib/quickstart.rst

+
+3. Navigate to ``docs/_build/html`` directory and open ``index.html`` file in your browser.
+
+Further Information


We should add parapgrah about modifing default nativeConfig used within the Scala Native project. The contributors should modify project/MyScalaNativePlugin instead of Build.scala. This way they would be able to quickly switch some tested options like usage of lto, release mode, etc.

added fcb9df6

- Remove specific version for LLVM and sbt - Move up sandbox section - Test on JVM for nir and tools

tanishiking · 2023-09-28T07:40:38Z

@LeeTibbert Thank you for the feedback!

The absolute top of the document should have at least a date, immediately after the title.

I agree, let's do this in another PR 👍

The first thing a potential Contributor should see/experience is a welcome, with
some indications that they can ask questions (describe where) for issues where
they have spent more than economical time trying to figure it out.

That sounds good! However, I don't know where's the place to ask question other than Github issues 🤔

sbtScalaNative/scripted in particular is one of those.

I'm not sure running scripted tests are advanced feature.
I understand we shouldn't write scripted tests if possible and it's better to write unit tests according to the test pyramid, but sbtScalaNative/scripted is a command to run scripted test which shouldn't be avoided.

LeeTibbert · 2023-09-28T12:43:36Z

@tanishiking

Given that PRs #3498 & #3512 have merged, I believe that the "test-all might not work" of this
section can be safely deleted. They are now no more fragile than any other unit-test. Note well
I do not say working (I think some of the scripted network tests are quietly borked. But then, they
are not loudly failing.)

LeeTibbert · 2023-09-28T12:48:07Z

docs/contrib/quickstart.rst

+- ``testsJVM3/test`` - run ``tests3/test`` on JVM
+- ``testsExtJVM3/test`` - run ``testsExt3/test`` on JVM
+- ``test-all`` - run all tests, ideally after ``reload`` and ``clean``
+  - Note: it is possible that some tests ran by `test-all` might not pass or compile on your system. We are currently working on stabilizing this test suite for our supported platforms


I recommend deleting this section. Two recently merged PR have made test-all more robust.

LeeTibbert · 2023-09-28T12:49:28Z

re: Date

Merged PR #3513 is a welcome advance. Given that we are in the discussion we are in because the
Contributor's Guide got shaggy and out-of-date, I recommend an explicit date at the top of the the
QuickStart doc also (even though it is down an index from the top level, which has date.)

LeeTibbert · 2023-09-28T12:50:44Z

Can the Contributor's Guide be titled & marked "OBSOLETE, for historical interest only".
This QuickStart is growing to be its replacement, and much more timely & useful.

LeeTibbert · 2023-09-28T13:03:37Z

re: sbtScalaNative/scripted

I believe that the author of a PR has wide latitude. They have taken the initiative. After a period of comment,
they have the next-to-final choice of design alternatives. The person doing the merge has the final choice.

I am also a believer that a bike shed actively shedding bikes from the rain (i.e. completed and at least approximately
correct) is far better than a bike shed of the perfect color, but undone and open to the weather.

That said:

In my recent work on Issue RuntimeException: failed tests #3486 I discovered that sbtScalaNative/scripted now takes, I believe, two parameters.
I believe the description sbtScalaNative/scripted run/backtrace needs an sbt version parameter. Do you know
if it has been run recently?
Yes, sbtScalanative/scripted needs to be described but it is pretty complicated and seldom used. Dropping
all that complexity in a section which is already long and complex already leads, at least me, to MEGO (My Eyes
Glaze Over). This is supposed to be a Quick Start. One of the great things about the Web & hyper-links is
that it is possible to have an Appendix or Advanced section and hyper-link to it. Finding the right
middle ground, and hyper-linking both more introductory and more advance topics is a goal, but hard,
especially for a community of readers with a wide distribution of skill & experience.

Your PR, your call, especially if it ships soon ;-)

LeeTibbert · 2023-09-28T13:07:37Z

Once this PR settles, I may take a run at the "Environment Setup" section of the "User's Guide".
The sections on required software sbt and LLVM software versions are in desperate need of
correction. The section on supported Scala Version is way out of date.

Time passes all to quickly and change comes with the dust.

At least this PR is making long overdue progress to a better place.

LeeTibbert · 2023-09-28T18:05:57Z

That sounds good! However, I don't know where's the place to ask question other than Github issues

Evidence that we have an opportunity for improvement ;-)

The place that I had in mind is the Discord scala-native channel.

The place where I come across it is the Scala Native GitHub Repository ReadMe.md
landing page. The description there
is all of a badge and assumes both that one knows what Discord is and can decode
the icon. We can do better.

So, with the consent of the folks in this discussion, I propose the following way forward:

I can add a brief bit of text describing Discord channel to the SN GitHub ReadMe landing page.
I can update the "Community" section of the ReadTheDocs latest
landing page. I would remove reference to gitter, and a link to the Chat section on the GitHub ReadMe
This document can add a link top the "Community" section of the ReadTheDocs landing page.

There are other way to stitch/link this together. Thoughts?

I think there is not a lot of work here and that some focused, surgical changes will re-pay the effort.

The SN Repository ReadMe.md has more problems than just the "Chat" section. Let's get this approximately
right and done first.

scala-native#3496 (comment)

it would be good to have this in a template, but let's do that in another Pull Request.

tanishiking · 2023-09-29T13:00:40Z

Can the Contributor's Guide be titled & marked "OBSOLETE, for historical interest only".

I believe current Contributors' guide is still "valuable" as a general guideline about writing valuable commit messages. While I agree it would be nice to have better name than "Contributor's Guide", but I don't have a good alternative off the top of my head ¯_(ツ)_/¯

The place that I had in mind is the Discord scala-native channel.

Oh, right 👍

I didn't think of this Discord server just because I'm not that active on that server (the conversation there is a bit too fast for me as slow English reader, since all the conversation is going on in 1 channel), but I think it's a good place to ask in a conversational form :)
I'll leave as is regarding the community in this PR for now, and leave it to you :)

* Add contributing quickstart guide * Add URL to sbt publish from "publish locally" * Update quick start guide * Remove specific version for LLVM and sbt * Mention MyScalaNativePlugin and scriptedBufferLog * Add built date to quickstart guide

LeeTibbert · 2023-10-05T23:48:56Z

@tanishiking Congratulations & thank you for getting this over the finish line.

* Add contributing quickstart guide * Add URL to sbt publish from "publish locally" * Update quick start guide * Remove specific version for LLVM and sbt * Mention MyScalaNativePlugin and scriptedBufferLog * Add built date to quickstart guide (cherry picked from commit 999df0d)

Add contributing quickstart guide

fd542a2

kyouko-taiga reviewed Sep 21, 2023

View reviewed changes

This was referenced Sep 21, 2023

RuntimeException: failed tests #3486

Open

SN 0.5.0 should update its minimal supported Clang & SBT versions #3497

Open

WojciechMazur reviewed Sep 24, 2023

View reviewed changes

tanishiking added 4 commits September 28, 2023 15:00

Add URL to sbt publish from "publish locally"

a82a1ee

Update quick start guide

b17677f

- Remove specific version for LLVM and sbt - Move up sandbox section - Test on JVM for nir and tools

Mention MyScalaNativePlugin and scriptedBufferLog

fcb9df6

Add disclaimer for test-all

1e57fea

tanishiking requested a review from WojciechMazur September 28, 2023 07:40

tanishiking mentioned this pull request Sep 28, 2023

Add a date when the document is built in the index #3513

Merged

LeeTibbert reviewed Sep 28, 2023

View reviewed changes

tanishiking added 2 commits September 29, 2023 21:32

Remove test-all disclaimer

5ce4b22

scala-native#3496 (comment)

Add built date to quickstart guide

57b7c44

it would be good to have this in a template, but let's do that in another Pull Request.

tanishiking mentioned this pull request Sep 29, 2023

Add last updated time to the footer or header of the document #3519

Closed

WojciechMazur approved these changes Oct 2, 2023

View reviewed changes

WojciechMazur merged commit 999df0d into scala-native:main Oct 2, 2023
76 checks passed

tanishiking mentioned this pull request Oct 12, 2023

Contributor's guide possibly out of date #3489

Closed


		It's convenient to run the ``sandbox`` project to verify the build works as expected.

		Publish Locally

		- ``nir3/test`` - run the unit tests for NIR
		- ``tools3/test`` - run the unit tests of the tools: ScalaNative backend


		3. Navigate to ``docs/_build/html`` directory and open ``index.html`` file in your browser.

		Further Information

Add contributing quickstart guide #3496

Add contributing quickstart guide #3496

Conversation

tanishiking commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyouko-taiga Sep 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekrich commented Sep 21, 2023

ekrich commented Sep 22, 2023

kyouko-taiga commented Sep 22, 2023

ekrich commented Sep 22, 2023

LeeTibbert commented Sep 22, 2023

LeeTibbert commented Sep 22, 2023

WojciechMazur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanishiking commented Sep 28, 2023

LeeTibbert commented Sep 28, 2023

Choose a reason for hiding this comment

LeeTibbert commented Sep 28, 2023

LeeTibbert commented Sep 28, 2023

LeeTibbert commented Sep 28, 2023

LeeTibbert commented Sep 28, 2023

LeeTibbert commented Sep 28, 2023

tanishiking commented Sep 29, 2023

LeeTibbert commented Oct 5, 2023

kyouko-taiga Sep 21, 2023 •

edited