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

Make sbt-github-actions work with Windows crlf line breaks #127

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Nov 30, 2022

On my Windows setup, GenerativePluginSpec/scripted tests are failing due to not handling Windows line breaks. This PR solves this by replacing Windows line breaks with Unix line breaks so that the rest of the logic works

Resolves: #126

Note that arguably a better solution for this problem is to use System.lineSeperator() rather \n, I tried to do this however its quite finicky and hence its also more risky but such an improvement can always be made in a future PR. Also as mentioned in the ticket this PR references the PR solves all of the issues on my Windows setup (i.e. sbt test scripted passes completely) however the github actions CI is still failing for what I assume are orthogonal reasons (so to verify this PR you need to run it on a Windows machine see #125)

@mdedetrich mdedetrich added the windows Issues specific to windows label Nov 30, 2022
@mdedetrich mdedetrich force-pushed the fix-generative-plugin-spec-for-windows branch 2 times, most recently from bdbe2c4 to 79c8ae6 Compare November 30, 2022 14:40
@mdedetrich mdedetrich changed the title Fix GenerativePluginSpec for Windows Fix Windows related failures due to line breaks Nov 30, 2022
@mdedetrich mdedetrich force-pushed the fix-generative-plugin-spec-for-windows branch from 79c8ae6 to 8fa69df Compare November 30, 2022 22:56
@mdedetrich
Copy link
Contributor Author

Just to be cautious, Ill merge this when @armanbilge looks at it.

@mdedetrich mdedetrich force-pushed the fix-generative-plugin-spec-for-windows branch 2 times, most recently from 2ab1442 to 5cac00c Compare December 1, 2022 23:31
@mdedetrich
Copy link
Contributor Author

mdedetrich commented Dec 1, 2022

So I just realized why my local Windows machine had failing tests for GenerativePluginSpec where as the CI did not, it turns out that sbt-github-actions detects if you have setup windows in githubWorkflowOSes and if so it will configure git with git config --global core.autocrlf false in the CI so that it will automatically convert windows crlf line ends to unix line endings.

Due to this I have updated the PR so to make the git auto.crlf an SBT plugin setting called githubWorkflowAutoCrlfWindows which defaults to true (i.e. current behaviour). For this projects CI specifically the PR sets the githubWorkflowAutoCrlfWindows value to false so that we can actually test that sbt-github-actions still continues to work even if we have windows vs unix line breaks.

While some can say what are the merits of the PR since you can just set auto.crlf on a Windows machine, I would personally argue that practically speaking sbt-github-actions shouldn't complain if the content of 2 generated workflows are exactly the same but with different line endings, in such a case the project would still work fine and it improves the user experience for Windows developers.

@mdedetrich mdedetrich force-pushed the fix-generative-plugin-spec-for-windows branch from 5cac00c to fab1103 Compare December 1, 2022 23:38
@mdedetrich mdedetrich force-pushed the fix-generative-plugin-spec-for-windows branch from fab1103 to edd3783 Compare December 1, 2022 23:43
@mdedetrich mdedetrich changed the title Fix Windows related failures due to line breaks Make sbt-github-actions work with Windows crlf line breaks Dec 1, 2022
@armanbilge
Copy link
Contributor

@mdedetrich first of all, thanks for all your work investigating this :) I'm glad you were able to minimize the issue to the core.autocrlf git config.

If I understand correctly, the changes here are not necessary for fixing CI, nor are they solving a user-reported issue. So personally I am cautious to make a change here, "just because".

I am also hesitant to use non-default settings for the plugin itself. Dog-fooding is an important method of testing, since it is the only time we can actually "integration test" this plugin in GHA. If we disable this feature just for this plugin, we will no longer be checking that the core.autocrlf works correctly in CI for all the projects that rely on it.

I would personally argue that practically speaking sbt-github-actions shouldn't complain if the content of 2 generated workflows are exactly the same but with different line endings, in such a case the project would still work fine and it improves the user experience for Windows developers.

Would this mean that Windows users can commit regenerated workflows that are identical except for changes in line breaks? It seems like this would allow for noisy git histories.

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Dec 2, 2022

Would this mean that Windows users can commit regenerated workflows that are identical except for changes in line breaks? It seems like this would allow for noisy git histories.

Yes although you could say that if a user is already regenerating a workflow then they are already committing said files at which point the noise is less of an issue. Unless a person actually triggers workflow generation with githubWorkflowGenerate then no crlf line breaks will be created in the workflow files, and there is no reason for a user to randomly run githubWorkflowGenerate unless sbt-github-actions complains (and it would only do this if the something changes).

If I understand correctly, the changes here are not necessary for fixing CI, nor are they solving a user-reported issue. So personally I am cautious to make a change here, "just because".

I would disagree here but there is one main reason why, afaik there isn't a way to enforce autocrlf when another project uses sbt-github-actions as a plugin. This means that if a person uses sbt-github-actions on a Windows machine and they don't have core.autocrlf set to true (its false by default) then sbt-github-actions will fail to work with non obvious errors due to mismatches in line breaks. I even got this problem when checking out the project on my Windows machine and running tests, it just fails from the get go. Right now when comparing the current workflow with a generated one its comparing exact contents of String which from a design perspective I don't think is entirely correct to begin with (there is an argument that it shouldn't even care about whitespace insofar that it doesn't change yaml's semantics but thats another point).

I am also hesitant to use non-default settings for the plugin itself. Dog-fooding is an important method of testing, since it is the only time we can actually "integration test" this plugin in GHA. If we disable this feature just for this plugin, we will no longer be checking that the core.autocrlf works correctly in CI for all the projects that rely on it.

True, but also given my previous statement that core.autocrlf is by default false (with no proper way of enforcing it for projects that use sbt-github-actions) one can easily claim that setting core.autocrlf to true in the current CI is a copout of not wanting to deal with differing line breaks for different operating systems. I can add a githubWorkflowAutoCrlfWindows set to true option in the scripted tests if we want to test both cases, would that alleviate your concerns?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
windows Issues specific to windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generative plugin spec failing on Windows (but not CI)
3 participants