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

CI setup #50

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

CI setup #50

wants to merge 12 commits into from

Conversation

matthiasbeyer
Copy link
Contributor

Hi,

we talked about this on mastodon. So here it goes.


This is a draft, because it has to be discussed first.

What I do here is more or less the default setup for my repositories. You might not want to have some parts of this setup, I don't know.

Let me explain the individual parts and then we can discuss what you want and what you don't want:

  • Workflows
    • CI
      • MSRV set to 1.67.0
      • Checks also with stable and beta rust toolchains
      • cargo-deny checks, but not enforced
        • These are not enforced, because they might break unrelated PRs. I'd rather have issues opened about cargo-deny failures (maybe automatically, not implemented in the patchset) than having a contributor contributing a typo and running into a cargo-deny vuln notification they don't know how to fix
      • cargo-fmt checks (obviously depends on Cargo fmt #48)
        • Only with one specific compiler version, so this has to be updated manually. This is also so that master does not break just because formatting fails after an update of the rust toolchain
      • cargo-clippy checks (obviously depends on Clippy fixes #49)
        • Only with one specific compiler version, so this has to be updated manually. This is also so that master does not break just because clippy fails after an update of the rust toolchain
      • cargo-test runs with MSRV, stable and beta
    • Commit-linting
      • This ensures good commit style, contributors use the Developers Certificate of Origin trailer and agree to the license terms
      • It also prevents "fixup!" (or similar) commits to be merged accidentially
    • Flake CI
      • Equal to "CI", but with the nix flake setup (read below)
      • With cachix configuration
        • Must be manually set up by you, or removed from the workflow
  • gitlint configuration (as noted above)
  • flake.nix setup
    • Developer shell setup based on rust-toolchain.toml (which is nightly right now)
    • Build setup using nix
      • Automatically does clippy, rustfmt checks on nix flake check

More to come:

  • If you decide to use the MPL-2.0 license, all files need to have a MPL license header (four lines IIRC). These will be checked with a dedicated action as well.

Bors setup

The workflow setup is prepared to work with bors as a mergebot.
I would strongly recommend setting up bors, as it makes merging and keeping master green so much simpler.
But of course that's totally up to you and I can remove the bors specific bits if you want me to!


Feel free to tell me what you think!
😆

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@netlify
Copy link

netlify bot commented Mar 10, 2023

Deploy Preview for thriving-beignet-855860 ready!

Name Link
🔨 Latest commit cefa407
🔍 Latest deploy log https://app.netlify.com/sites/thriving-beignet-855860/deploys/640c36ae8627570008773c6a
😎 Deploy Preview https://deploy-preview-50--thriving-beignet-855860.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@zesterer
Copy link
Owner

For the time being, I'd prefer to just have a basic cargo check CI pass rather than deny warnings + clippy + fmt. While I think those things are useful for a production project, there are many unused things in the codebase that will trigger many warnings (both from rustc and clippy): and that's good! I want to know when something is unused, it's a good reminder for me to implement it. 😀 Additionally, rustfmt - while nice as a consistent lowest common denominator - tends to wreck havoc with very dense code, actively making it more difficult to reason about. I fear what it would do to the type checker (analysis/src/infer.rs) would be pretty horrid.

@matthiasbeyer
Copy link
Contributor Author

Ok, I'll remove the bits that you deem unnecessary for now, but in seperate patches so we can easily git-revert them when the time comes.

I will keep the bors compatibility for now, if that's ok.

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@zesterer
Copy link
Owner

Thanks! Much appreciated.

@matthiasbeyer matthiasbeyer marked this pull request as ready for review March 11, 2023 09:54
@matthiasbeyer
Copy link
Contributor Author

So, if you want to set up bors, I can help you with that if you haven't done it before. If not, I can remove the bits that are required for bors.

@matthiasbeyer
Copy link
Contributor Author

matthiasbeyer commented Mar 11, 2023

Another thing: As far as I can see, there's no mechanism in the repository to test all that .tao files that are in the repository, is there? I see that there's some testing setup in compiler/tests, but there's a lot more .tao files in the repository than are tested there...

Would you be interested in a test setup that verifies that the existing .tao files work (with or without CI integration)?

@zesterer
Copy link
Owner

zesterer commented Mar 11, 2023

So, if you want to set up bors, I can help you with that if you haven't done it before. If not, I can remove the bits that are required for bors.

I think keeping things simple is fine for now. I really want to emphasise the extent to which this project is just something I'm mucking around with, and it's probably going to take quite a lot of work to push to to a point where people might actually want to use (and depend) on it.

Another thing: As far as I can see, there's no mechanism in the repository to test all that .tao files that are in the repository, is there? I see that there's some testing setup in compiler/tests, but there's a lot more .tao files in the repository than are tested there...

I did make a much earlier attempt at that as you mention, although it was pretty shoddy. I definitely think that having a set of tests specifically for a suite of minimal tests to catch regressions is very useful (and perhaps testing of more official examples too), but everything in test_cases/ is pretty much just me playing around with ideas and IMO shouldn't be expected to compile. I'm sure eventually I'll end up clearing that out and using a .gitignore-d local directory or something instead. As I say, personal project so far so a lot of the repository is a product of that fact.

That said, I want to be careful about over-testing. The language is still in flux and syntax/semantics/core/std change on virtually every commit. I think the worst thing for the project at this stage would be a situation where making a change takes 10 minutes and then fixing all of the tests to account for it takes 30 minutes.

Additionally, features often get removed: for example, the language previously had anonymous sum types (union types) but I removed them because they were muddying the type checker in awkward ways. I want to reimplement them later, but it's this sort of thing that would likely become an enormous maintenance burden without much material benefit if we were to over-test things.

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.

None yet

2 participants