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

Checking on windows with configure.ac script will always result in error due to CRLF issues #19

Open
caiohamamura opened this issue May 28, 2024 · 11 comments

Comments

@caiohamamura
Copy link

caiohamamura commented May 28, 2024

Even though I already made sure that I submitted the configure.ac with LF only line endings (by git ls-files --eol, see attached image) this will still be converted due to incorrect configuration within the git's checking process and result in R CMD check warnings which will lead to error.
 
image

See my repository job: https://github.com/caiohamamura/gdalBindings-r/actions/runs/9275022762/job/25518759238. Every other machines are passing gracefully.

@gaborcsardi
Copy link
Contributor

The default GitHub configuration uses CRLF line endings on Windows, see e.g. actions/checkout#135

You can probably work around this by adding a .gitattributes file, see https://docs.github.com/en/get-started/getting-started-with-git/configuring-git-to-handle-line-endings

@caiohamamura
Copy link
Author

Yes, indeed this does the job, but should the developer always have to have that concern just for auto checking? Can't we just add before the checkout action by default a step for:

 - run: git config --global core.autocrlf false

Would that hurt anything else?

@gaborcsardi
Copy link
Contributor

Would that hurt anything else?

Possibly.

But it is a little weird that this has come up now, I have never heard of this being an issue, and these (similar) workflows are used by thousands of repos. OK, most don't have a configure.ac file, but some do. So let me look why this is an issue for you, and why it wasn't an issue until now.

@caiohamamura
Copy link
Author

Looking inside some packages like sf, they do use the .gitattributes to avoid that: https://github.com/r-spatial/sf/blob/main/.gitattributes.

@caiohamamura
Copy link
Author

caiohamamura commented May 29, 2024

I've edited my actions yaml to include - run: git config --global core.autocrlf false and the checking warning indeed goes away.

https://github.com/caiohamamura/gdalBindings-r/blob/8736d328d08511a3e0d6570c20a9c3fd513f2b94/.github/workflows/rhub.yaml#L81

I doubt that including this step will harm anything else as all it does is configure git to not modify the line endings when checking out code from repositories. Actually, in my opinion this is even more consistent, because this will allow to test if indeed the developer commit by mistake any files with offending line endings. As it is now using the default windows git behavior the package checkings for line endings is useless and misleading, because it is actually testing git's behavior instead of the actual code.

@DavidRConnell
Copy link

But it is a little weird that this has come up now, I have never heard of this being an issue

For the record, I was also getting a warning specifically for configure.ac on Windows checks but outside of using these rhub workflows. I was using checks based on r-lib's workflows. Adding:

- name: Fix line-endings
  if: runner.os == 'Windows'
  run: dos2unix configure.ac

Prevents the warning and may be less likely to affect anything else than changing the git config. (In terms of this project, probably replace runner.os with ${{ matrix.config.os }})

I do not know why this is specific to configure.ac files though.

@MichaelChirico
Copy link

I'm also running into this now... Still not sure how to resolve it.

Rdatatable/data.table#6379

I have a GHA that looks for files with CR that works locally, but fails on GHA because the line endings are converted by the actions/checkout step.

Probably there's a smarter way to go about this, but sharing as another affected use case.

@gaborcsardi
Copy link
Contributor

I think the best fix for this is indeed a .gitattributes file, because that'll ensure that all Windows users that clone your repo will get the correct line endings, irrespectively of their local settings.

@MichaelChirico
Copy link

Makes sense... my hangup is that we also keep a bunch of CSV (etc) files around for testing purposes, I don't want to mess around with their line endings lest we end up testing the wrong thing.

I'm also confused because we already have * -text as the first line of our .gitattributes, which is ostensibly being ignored by actions/checkout:

https://github.com/Rdatatable/data.table/blob/master/.gitattributes

@caiohamamura
Copy link
Author

caiohamamura commented Aug 23, 2024

I think the best fix for this is indeed a .gitattributes file, because that'll ensure that all Windows users that clone your repo will get the correct line endings, irrespectively of their local settings.

This does work, however the configure.ac will probably never be used anyway by Windows users as we need to ship the configure.win or configure.ucrt anyway, most Windows users will never ever work with autotools as that's very specific for developers and if they ever get any trouble cloning the configure.ac with CRLF line endings they for sure will know how to get that fixed.

But as I said I still consider this as a bad behavior of the checking process, as it is checking the default git behavior of each platform it is being tested on by the CI instead of the actual code pushed to GitHub.

@gaborcsardi
Copy link
Contributor

will probably never be used anyway by Windows users

I meant people and services that check out your repo on Windows, and then run R CMD check on it. They might get a check error, depending on their default git setup.

You can fix this in .gitattributes, and then it will work everywhere.

Or you'll have to ask everybody else to fix it for you, e.g. here at r-hub/actions, in r-lib/actions, on the Bioconductor servers if your package is on Bioconductor, etc.

According to actions/checkout#226 (comment) we would need something like

      - name: Prepare git
        run: |-
          git config --global core.autocrlf false
          git config --global core.eol lf

for the general case, which is actually not that great, because it also converts CRLF files to LF (I think, but fixme), which might not be what you want. It would definitely break at least one repo of mine.

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

No branches or pull requests

4 participants