r-hub / actions

GitHub Actions for R-hub
MIT License
21 stars 4 forks source link

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

Open caiohamamura opened 3 months ago

caiohamamura commented 3 months ago

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 commented 3 months ago

The default GitHub configuration uses CRLF line endings on Windows, see e.g. https://github.com/actions/checkout/issues/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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

caiohamamura commented 3 months ago

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 commented 1 month ago

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 commented 2 weeks ago

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

https://github.com/Rdatatable/data.table/pull/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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 https://github.com/actions/checkout/issues/226#issuecomment-854736025 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.