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

Migrate to sbt-typelevel and self-host #120

Merged
merged 12 commits into from
Jan 18, 2022

Conversation

armanbilge
Copy link
Member

Some comments below.

Comment on lines 48 to 50
List("doc"),
name = Some("Build docs"),
cond = Some("matrix.ci == '' || matrix.ci == 'ciJVM'")),
Copy link
Member Author

Choose a reason for hiding this comment

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

So, this cond here is a bit annoying.

Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying to account for CI matrices with a JVM/JS axis here. There might be a better way to do this.

We only want to run doc once per platform, but we don't know if there are platforms and if so how they are setup. E.g. we might want to do project rootJVM; doc and project rootJS; doc in their appropriate jobs. But that seems hard, since not all project have rootJVM and rootJS.

So, what I did was try to run doc conditionally:

  1. matrix.ci == 'ciJVM' we are running in a JVM job in a crossed matrix, so we should run doc
  2. matrix.ci == '' The ci key is not defined, so the matrix is not crossed, and we should run doc

Of course, we could run this step on all the jobs, but it would be redundant and time-consuming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing typelevel/sbt-typelevel#61 would fix this insanity.

githubWorkflowBuildMatrixFailFast := Some(false),
githubWorkflowArtifactUpload := false,
Copy link
Member Author

Choose a reason for hiding this comment

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

Any reason we don't like this?

Copy link
Member

Choose a reason for hiding this comment

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

I remember it being an irritant, but don't remember why. I'm happy to try without it.

Comment on lines 54 to 55
List("unusedCompileDependenciesTest"),
name = Some("Check unused compile dependencies")),
Copy link
Member Author

Choose a reason for hiding this comment

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

This is something not currently covered in sbt-typelevel. I'm a bit confused, are projects actually using this? If so I should add it back.

Copy link
Member

Choose a reason for hiding this comment

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

http4s should be. I think it's good POM hygiene, but it's one of my less popular opinions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've never used it but seems like a good idea. I don't see it in the http4s CI rn:
https://github.com/http4s/http4s/blob/0671d49d246639e3edad89d7f8e1ba979d9a7658/.github/workflows/ci.yml#L100-L125

Copy link
Member

Choose a reason for hiding this comment

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

We had the explicit dependencies check in addition to the unused dependencies check. The former was very controversial, but I'd like to reinstate the latter.

@armanbilge armanbilge mentioned this pull request Jan 8, 2022
2 tasks
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Maybe it's good to have a common org shim as we think about the Great Schism, but this plugin has shrunk to the point that I question whether it's still useful... which is a good thing.


- name: Run tests
run: sbt ++${{ matrix.scala }} test
- run: sbt ++${{ matrix.scala }} ci
Copy link
Member

Choose a reason for hiding this comment

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

Now that I see it from a workflow perspective, I'm not sure I consider this progress. It is now harder to see on what step a build fails.

Copy link
Member Author

@armanbilge armanbilge Jan 9, 2022

Choose a reason for hiding this comment

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

Opened typelevel/sbt-typelevel#60 to think about this one.

Comment on lines 54 to 55
List("unusedCompileDependenciesTest"),
name = Some("Check unused compile dependencies")),
Copy link
Member

Choose a reason for hiding this comment

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

http4s should be. I think it's good POM hygiene, but it's one of my less popular opinions.

Comment on lines 48 to 50
List("doc"),
name = Some("Build docs"),
cond = Some("matrix.ci == '' || matrix.ci == 'ciJVM'")),
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand it.

@armanbilge
Copy link
Member Author

but this plugin has shrunk to the point that I question whether it's still useful

We should possibly upstream more stuff from http4s proper to here, for example the laika config. I've currently duplicated it in the http4s-dom build to keep the theme consistent.

@rossabaker
Copy link
Member

Yeah, site configs are one of the last remaining big chunks in the modern g8 templates. I'd love to see an easy way to start a Laika site, and encourage that in new project templates.

@armanbilge
Copy link
Member Author

armanbilge commented Jan 10, 2022

I think I heard of this happening to someone once, and it's baffling. Edit: here maybe: http4s/http4s#5313 (comment).

[error]       - name: Check headers
[error]                     ^ (different character)

@armanbilge
Copy link
Member Author

Oh, I know what's going on: non-determinism in the order settings are enabled actually. Seems to be causing the headers and formatting checks to swap around?

@armanbilge
Copy link
Member Author

armanbilge commented Jan 10, 2022

I wonder if it's related to the version of sbt installed on the system (not defined in the build.properties). Because it seems to be deterministic/consistent across runs per machine, just different across machines.

The GH runner ships with sbt 1.5.7 (according to this) and my other local where I fixed the workflow has sbt 1.5.0 installed. My dev machine has 1.6.1 on it.

Edit: nope, not this.

@rossabaker
Copy link
Member

WTF. Is that not an ordered collection in the GitHub plugin?

@rossabaker
Copy link
Member

Oh, yikes. I don't know how to linearize that without one plugin knowing about the other.

Those plugins don't really do much. I wouldn't mind if these were baked into the opinionated Typelevel plugin, and anybody who doesn't want them can filter the steps, override the tasks, or just use the lower level plugin.

@armanbilge
Copy link
Member Author

Yep, I got a bit too ambitious there with all the separate plugins. Too much pain for little gain. It's already plenty flexible.

@armanbilge armanbilge linked an issue Jan 10, 2022 that may be closed by this pull request
@armanbilge
Copy link
Member Author

I think this is ready.

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.

Create default matrix exclusions
2 participants