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

Setup CI #13

Merged
merged 8 commits into from
Sep 25, 2024
Merged

Setup CI #13

merged 8 commits into from
Sep 25, 2024

Conversation

DigitalBrains1
Copy link
Member

@DigitalBrains1 DigitalBrains1 commented Sep 23, 2024

This also establishes a proper cabal.project and fixes compilation with Clash 1.8.1, although there are currently still bugs in that. Before we release clash-cores, we need to either fix the following bugs, release Clash 1.10.0 and use that, or disable the problematic cores:

It was not enough to use a global unchanging cabal.project and amend that with a cabal.project.local in CI: it did not allow altering the things that needed to be altered. So I had to overwrite part of cabal.project itself.

@DigitalBrains1
Copy link
Member Author

Finally, let's do some bikeshedding. I moved this to its own message so it doesn't detract from the main points in the cover letter.

The following stanza illustrates the current multi-coloured shed in all its glory:

test-suite unittests
  type: exitcode-stdio-1.0

Here we decided to simply write multiple words together (I think this was a Dunglish issue where a native Dutch speaker incorrectly writes multiple words, unit tests, as a single word, unittests). However, Cabal can't make up its mind: it says test-suite, combining multiple words with a dash, and exitcode where it just strings multiple words together. Or, rather, two words together. It then has another dash because of course.¹

However, it does explicitly spell test-suite, which makes clash-testsuite rather weird because you're mixing multiple spellings in the same .cabal file. Here, I opted to call the test suite package clash-cores-test-suite which is quite a mouth full and has lots of dashes.

And another bikeshed. I dislike the .yml extension for YAML files. The only reason for not using .yaml seems to be compatibility with filename restrictions that haven't existed for three decades in any of the platforms we use. Currently, I do use .gitlab-ci.yml because GitLab needs configuration to change that name, but I use .yaml for the GitLab include files as the doc explicitly says .yml and .yaml are the two allowed extensions. Frankly, I'm inclined to configure the GitLab repo to look for .gitlab-ci.yaml, but I kept it at the default for now.

What do people think of the spellings? test-suite? .yaml? Let the bikeshedding beginnnnn!

¹ We could "fix" this inconsistency by bumping cabal-version to 3.8, where the type property becomes the optional default, so we could remove that :-D.

@martijnbastiaan
Copy link
Member

martijnbastiaan commented Sep 24, 2024

Bikeshedding:

  • yaml
  • testsuite: integration-tests? hdl-tests?
  • unittests: unit-tests though this one pains me because we already Dunglish it everywhere.

test-suite/Main.hs Outdated Show resolved Hide resolved
@DigitalBrains1
Copy link
Member Author

DigitalBrains1 commented Sep 24, 2024

Quote with reply:

Bikeshedding:

  • yaml
    yes. I'll make it .gitlab-ci.yaml and configure GitLab accordingly
  • testsuite: integration-tests? hdl-tests?
    hdl-tests makes sense to me
  • unittests: unit-tests though this one pains me because we already Dunglish it everywhere.
    I erred on the side of copy-pasting, but I do like internal consistency, I like unit-tests actually. And doc-tests, right? Right, not doctests. The only place I see that as separate words is in the module path (Test.DocTest), everywhere else people seem to consider this a single word.

@DigitalBrains1 DigitalBrains1 force-pushed the peter/clean-setup-ci branch 2 times, most recently from b7c1322 to c8dcbda Compare September 24, 2024 15:59
DigitalBrains1 and others added 6 commits September 24, 2024 19:02
The suite of HDL tests needs to be in its own package because it depends
on `clash-testsuite` which is not on Hackage. We need to be able to
release `clash-cores` to Hackage without it depending on
`clash-testsuite` directly.
Note that it appears that the Cabal store does not need to be first in
the env var `GHC_PACKAGE_PATHS` (it is automatically appended because
the env var ends in a `:`). The fact that the path was wrong for GHC 9.8
yet it worked fine clued us in on it being superfluous.
As of GHC 9.8, that function gives a warning
The HDL tests fail for several cores. The only thing done about this for
now is to disable those tests and make a GitHub issue for each of them.
Before release, the bugs should be fixed or the cores removed from the
build for Clash 1.8.
@DigitalBrains1 DigitalBrains1 force-pushed the peter/clean-setup-ci branch 3 times, most recently from f70286d to b5da0c3 Compare September 24, 2024 18:46
@martijnbastiaan
Copy link
Member

Yeah doctest seems to have become one word.

@DigitalBrains1 DigitalBrains1 merged commit d6e3269 into main Sep 25, 2024
1 of 2 checks passed
@DigitalBrains1 DigitalBrains1 deleted the peter/clean-setup-ci branch September 25, 2024 10:35
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.

4 participants