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.

Sign up for GitHub

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

Jump to bottom

[#454] Implement cross-platform GitHub Actions. #511

Merged
merged 8 commits into from
Oct 23, 2020
Merged

[#454] Implement cross-platform GitHub Actions. #511

merged 8 commits into from
Oct 23, 2020

Conversation

gdziadkiewicz
Copy link
Collaborator

@gdziadkiewicz gdziadkiewicz commented Oct 11, 2020 •
edited
Loading

Fix #454

hint-man[bot]
hint-man bot reviewed Oct 11, 2020
View reviewed changes
Copy link
Contributor

@hint-man hint-man bot 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 know why your PR is still not approved? Because I chose not to approve it. But they will.

summoner-cli/examples/cabal-full/.github/workflows/ci.yml Outdated Show resolved Hide resolved
summoner-cli/src/Summoner/Template/GitHub.hs Outdated Show resolved Hide resolved
@gdziadkiewicz @hint-man
Update summoner-cli/src/Summoner/Template/GitHub.hs
7592a23 
Co-authored-by: hint-man[bot] <44720633+hint-man[bot]@users.noreply.github.com>
@gdziadkiewicz
Copy link
Collaborator Author

Repo created using Summoner with this change: https://github.com/gdziadkiewicz/NowyTestowy

@gdziadkiewicz gdziadkiewicz marked this pull request as ready for review October 11, 2020 15:02
@chshersh chshersh added CI CI tools support; project CI; build with different GHC, tools generated project Files, folder generation by the summoner hacktoberfest-accepted labels Oct 14, 2020
chshersh
chshersh reviewed Oct 14, 2020
View reviewed changes
Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

@gdziadkiewicz Nice work! I left some comments and some things to keep in mind.

The implementation looks good 👍 But a few more patches required 🙂

summoner-cli/src/Summoner/Project.hs Show resolved Hide resolved
summoner-cli/src/Summoner/Template/GitHub.hs Outdated Show resolved Hide resolved
summoner-cli/src/Summoner/Template/GitHub.hs Outdated Show resolved Hide resolved
summoner-cli/src/Summoner/Template/GitHub.hs Outdated Show resolved Hide resolved
summoner-cli/src/Summoner/Template/GitHub.hs Show resolved Hide resolved
@gdziadkiewicz gdziadkiewicz force-pushed the #454-Implement_cross-platform_GitHub_Actions branch 2 times, most recently from 33a3982 to 6da1b70 Compare October 14, 2020 22:02
@chshersh
Copy link
Contributor

Hi @gdziadkiewicz! I see that you've also moved everything to Cabal-3.0. This is a separate big issue, and ideally should be done separately. For now, the only thing required for GitHub Actions is to use cabal 3.0 in .github/workflows/ci.yml file. Can you please revert the commit with all changes to Cabal 3.0 and fix only the GitHub Actions workflow? Moving everything to Cabal 3.0 is tricky and requires more thoughtful consideration and testing, and we are planning to do it after some other fixes.

@gdziadkiewicz gdziadkiewicz force-pushed the #454-Implement_cross-platform_GitHub_Actions branch from 6da1b70 to 4035c0c Compare October 18, 2020 11:32
@gdziadkiewicz
Copy link
Collaborator Author

@chshersh I reverted cabal 3.0 changes, and now I'm working on remaining issues.

@gdziadkiewicz
Copy link
Collaborator Author

Committed changes to https://github.com/gdziadkiewicz/NowyTestowy

@gdziadkiewicz
Copy link
Collaborator Author

gdziadkiewicz commented Oct 19, 2020 •
edited
Loading

I believe that this is ready to be reviewed once again :)

@gdziadkiewicz
Copy link
Collaborator Author

@chshersh While working on GitHub Actions for my Haskell project, I found out that actions/setup-haskell will install unneeded Cabal and global GHC if configured the way we do it here/ in your article. I checked the documentation for it, and it confirms that this is expected. The missing part to avoid having it installed is to add:

stack-no-global: true
enable-stack: true

to the stack job.

What's your opinion on adding that?

gdziadkiewicz
gdziadkiewicz commented Oct 20, 2020
View reviewed changes
summoner-cli/src/Summoner/Default.hs Outdated
@@ -42,6 +43,10 @@ defaultGHC = maxBound
defaultCabal :: Text
defaultCabal = "2.4"

-- | Default version of the Stack.
defaultStack :: Text
defaultStack = "2.3.3"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

2.5.1 was released since I did that. Should I update it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to 2.5.1 .

chshersh
chshersh approved these changes Oct 21, 2020
View reviewed changes
Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Looking good! 👍 I hope I didn't miss anything 🤞 Waiting for the review from @vrom911, she can review very carefully and notice anything suspicious.

I found out that actions/setup-haskell will install unneeded Cabal and global GHC if configured the way we do it here
What's your opinion on adding that?

Can you elaborate a bit more on that? The fact that setup-haskell installs global GHC is plus ➕ It can do it faster and more efficiently than Stack (either via ghcup or even using already provided by environment version). So, in Stack build we use the --system-ghc flag to tell Stack to use already installed GHC and not install it on its own.

appveyor.yml
@@ -17,7 +17,7 @@ cache:

environment:
STACK_ROOT: C:\sr
STACK_VERSION: 2.1.1
STACK_VERSION: 2.5.1
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that Stack 2.5.1 is on Chocolatey, so this should work 👍
Just a lot of places needed to be look before bumping versions of tools everywhere...

summoner-cli/src/Summoner/Template/GitHub.hs Show resolved Hide resolved
@chshersh chshersh mentioned this pull request Oct 21, 2020
Closed
@gdziadkiewicz
Copy link
Collaborator Author

I missed the use of --system-ghc and was under the impression that we don't gain anything from installing GHC and Cabal during Stack job.

vrom911
vrom911 reviewed Oct 22, 2020
View reviewed changes
Copy link
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

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

Great stuff, thanks so much for working on this. And thanks a lot for your patience 🤗

summoner-cli/src/Summoner/Template/GitHub.hs Outdated Show resolved Hide resolved
summoner-cli/src/Summoner/Template/GitHub.hs Outdated Show resolved Hide resolved
summoner-cli/src/Summoner/Template/GitHub.hs Outdated Show resolved Hide resolved
summoner-cli/src/Summoner/Template/GitHub.hs Show resolved Hide resolved
@chshersh
Copy link
Contributor

Btw, @gdziadkiewicz, are you participating in Hacktoberfest this year? The Hacktoberfest rules has changed, and now it's opt-in for all repositories, but we haven't added the hacktoberfest topic to Summoner, as we are very busy with Learn4Haskell. But if you are participating, let us know, so your contributions to Summoner would count towards 4 PRs goal 🙂

@gdziadkiewicz
Copy link
Collaborator Author

@chshersh I do participate in Hactoberfest, but I already managed to make 4 contributions :) I implemented the fixes as discussed. @vrom911 could you check if after the changes it's good to go from your perspective?

vrom911
vrom911 approved these changes Oct 22, 2020
View reviewed changes
Copy link
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

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

Looks fantastic! ✨
Thanks a lot for working on this one 💪🏼

@vrom911 vrom911 merged commit e632eab into kowainik:master Oct 23, 2020
@gdziadkiewicz gdziadkiewicz deleted the #454-Implement_cross-platform_GitHub_Actions branch October 23, 2020 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hint-man hint-man[bot] hint-man[bot] left review comments

@chshersh chshersh chshersh approved these changes

@vrom911 vrom911 vrom911 approved these changes

Assignees

@gdziadkiewicz gdziadkiewicz

Labels
CI CI tools support; project CI; build with different GHC, tools generated project Files, folder generation by the summoner hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement cross-platform GitHub Actions
3 participants
@gdziadkiewicz @chshersh @vrom911