sbt-github-actions icon indicating copy to clipboard operation
sbt-github-actions copied to clipboard

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

Open mdedetrich opened this issue 3 years ago • 4 comments

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: https://github.com/sbt/sbt-github-actions/issues/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 https://github.com/sbt/sbt-github-actions/pull/125)

mdedetrich avatar Nov 30 '22 13:11 mdedetrich

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

mdedetrich avatar Dec 01 '22 09:12 mdedetrich

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 avatar Dec 01 '22 23:12 mdedetrich

@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.

armanbilge avatar Dec 01 '22 23:12 armanbilge

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?

mdedetrich avatar Dec 02 '22 00:12 mdedetrich