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

Add a basic GitHub Actions CI script #20

Merged
merged 6 commits into from
Apr 16, 2024
Merged

Add a basic GitHub Actions CI script #20

merged 6 commits into from
Apr 16, 2024

Conversation

arichardson
Copy link
Member

@arichardson
Copy link
Member Author

Sounds like the compiler in this job is more strict. @PeterRugg @gameboo are those warnings worth fixing?

@PeterRugg
Copy link
Collaborator

Hmm, unless we can configure the compiler we normally use to throw the same errors, this would just be frustrating. That said, it might be worth fixing them. I don't know if we're actually confusing variables anywhere.

@arichardson
Copy link
Member Author

Hmm, unless we can configure the compiler we normally use to throw the same errors, this would just be frustrating. That said, it might be worth fixing them. I don't know if we're actually confusing variables anywhere.

I think you tell it to use a given version. Alternatively there also seems to be https://github.com/haskell/actions/tree/main/setup which allows more configuration but requires adding manual build steps instead of just relying on stack.

@gameboo
Copy link
Member

gameboo commented Feb 2, 2023

I've only used github actions doing cabalisms and ghcisms before, but there may be useful things to fix here anyways. I'll see if I can reproduce locally.

@arichardson arichardson force-pushed the github-ci branch 2 times, most recently from cf38208 to bc030b2 Compare February 2, 2023 10:53
@arichardson
Copy link
Member Author

I've only used github actions doing cabalisms and ghcisms before, but there may be useful things to fix here anyways. I'll see if I can reproduce locally.

The following works for me stack clean && stack build --pedantic --ghc-options -fno-warn-unused-matches --ghc-options -fno-warn-name-shadowing
I guess there is a similar flag to --pedantic for cabal.

@gameboo
Copy link
Member

gameboo commented Feb 2, 2023

hehe... trying to install stack... it defaults to my nfs... I am out of quota... yay!

@gameboo
Copy link
Member

gameboo commented Feb 2, 2023

so looking at some of the annotations:

XXX is not used by any defined root

sure, but, I mean... So what? I am still exporting this out of my library module... Not sure why this gives a red cross there...

EDIT: is there a way to tell stack that I want this?

@arichardson
Copy link
Member Author

arichardson commented Feb 2, 2023

so looking at some of the annotations:

XXX is not used by any defined root

sure, but, I mean... So what? I am still exporting this out of my library module... Not sure why this gives a red cross there...

EDIT: is there a way to tell stack that I want this?

We can drop the weeder bits from this CI run, I just thought it might be helpful to find unused functions. I think we can also add additional roots to tell it that those functions should be ignored.
See https://github.com/ocharles/weeder#readme

@gameboo
Copy link
Member

gameboo commented Feb 2, 2023

oo fancy.

Let's drop this for now but I am having a pass fixing a bunch of warnings and I'll push on the branch if that's ok?

@gameboo
Copy link
Member

gameboo commented Feb 2, 2023

I think we can also add additional roots to tell it that those functions should be ignored.

It'd be nice to do this at some point

@arichardson
Copy link
Member Author

oo fancy.

Let's drop this for now but I am having a pass fixing a bunch of warnings and I'll push on the branch if that's ok?

That sounds good! I can drop the "weeder" part of this PR?

@gameboo
Copy link
Member

gameboo commented Feb 3, 2023

yeah let's drop it for now
I just finished building with full pedantic except for the orphan class thing but there is no real story to tackle that one in general... We had discussion already on where this code ought to live but from what I remember we don't think it should go near the type. so, -fno-warn

@gameboo
Copy link
Member

gameboo commented Apr 15, 2024

@arichardson I reverted my commits which seemed to conflict with a recent master and updated the ci script to remove the weeder, and add a build step as well. I will merge this PR and we can add a weeder at some point if we want.

I'll probably also have a pass at re implementing these commits I reverted at some point...

@gameboo gameboo self-assigned this Apr 15, 2024
@gameboo gameboo requested a review from PeterRugg April 15, 2024 18:06
Copy link
Member

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

Do you mind removing the commits from the PR instead of reverting them? This keeps the history of the main branch cleaner.

You can do this with the command:
git rebase -i HEAD~12

Then dropping the commits and the reversion as needed.

@gameboo
Copy link
Member

gameboo commented Apr 16, 2024

Sure thing. I'll create a branch somewhere though, because I am keen to hold on to the changesets somewhere (I'll eventually want to implement these).

@gameboo
Copy link
Member

gameboo commented Apr 16, 2024

@marnovandermaas done!

@PeterRugg PeterRugg merged commit c85769c into master Apr 16, 2024
2 checks passed
@arichardson arichardson deleted the github-ci branch April 16, 2024 23:49
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