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

Adding the .gitattributes file so it can make it easier for Windows u… #9905

Closed
wants to merge 3 commits into from
Closed

Adding the .gitattributes file so it can make it easier for Windows u… #9905

wants to merge 3 commits into from

Conversation

jp-tosca
Copy link
Contributor

What this PR does / why we need it:

Adding the .gitattributes file so it can make it easier for Windows users to get going on the code by just adding a file to the Git repository.

Which issue(s) this PR closes:

Special notes for your reviewer:

https://groups.google.com/g/dataverse-community/c/dGduT2T8F7Y

Suggestions on how to test this:

Download the project from the original repo
Create a windows installation and set the core.autocrlf to false.
The development of the containers should fail.
Delete the instalation
Set core.autocrlf=input to input or true
The development of the containers should be successful.
set the core.autocrlf to false again
download from this fork
the development should be successful

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No

Is there a release notes update needed for this change?:
Change on the documentation at https://guides.dataverse.org/en/latest/developers/windows.html this step would not be necessary

Additional documentation:
https://www.git-scm.com/docs/gitattributes

…sers to get going on the code by just adding a file to the Git repository.

Issue #9894
@poikilotherm
Copy link
Contributor

@jp-tosca I switched your line to not only be including shell scripts, but all text files. If we will be having more Windows based devs around, we should make sure the CRLF problem is solved for all files checked in to avoid nasty errors.

@pdurbin I did a normalization of files as well - we do have quite a lot of files in our codebase that are CRLF. Editors and IDEs these days should be fine to do autodetection, so if you think it's not worth it, we can revert / delete 51dd829.

@pdurbin
Copy link
Member

pdurbin commented Sep 13, 2023

I'm confused. The original fix from @jp-tosca is this:

*.sh text eol=lf

But then @poikilotherm changed it to this:

* text=auto

It seems pretty different. Either one works? It doesn't matter which on we merge? I can't test either one because I don't have a Windows box.

Also, yeah, I think I'd rather solve the problem at hand than change an additional 32 files.

We can start a new topic in Zulip if we want. Under the dev stream, I'd say.

@jp-tosca
Copy link
Contributor Author

I think that * text=auto just defaults to the default behavior 🤣 where people have to set the configuration on their machine with git config --global core.autocrlf input

Both solutions work but the idea of this change was that that every person who pulls out the configuration needs to change their configuration to input or false to turn off the conversion, this is an additional step that can be omitted, also if someone pulled out the project before reading the documentation (🙄), and they change the configuration after the files will be converted already.

This doesn't affect linux users since the conversion happens to Windows and docker runs a linux instance, so basically on linux is - linux (project) -> linux (dev) - > linux(docker) and for widnows is linux (project) -> windows (dev) - > linux(docker)

let me know your thoughts, as for more additional files we can add them as they find conflicting, mostly .sh files are the ones that give trouble since they are executed on the docker (same logic linux (project) -> windows (dev) - > linux(docker) ). I could think that if we have more resources executed by the server, like an example would be a JS file executed by Node.JS then it would need this.

If I or someone else find more resources that need to be added to this file, we can add them to the .gitattributes

@pdurbin
Copy link
Member

pdurbin commented Sep 13, 2023

Here's a comment from @ethomson I'm copying from from #3927 (comment)

It seems relevant. And here's his blog post he mentioned: https://www.edwardthomson.com/blog/git_for_windows_line_endings.html


👋 One thing that I didn't mention in that particular blog post is that .gitattributes gives you more fine-grained control over how your files are handled. For example, if you wanted most of your files to have native line endings (* text=auto) you could also override this on a per-file basis to have a particular set of line endings. eg:

* text=auto
install eol=lf

will ensure that the install file always has Unix-style line endings when checked out.

@poikilotherm
Copy link
Contributor

poikilotherm commented Sep 13, 2023

I think that * text=auto just defaults to the default behavior 🤣 where people have to set the configuration on their machine with git config --global core.autocrlf input

Nope.

Quoting from https://www.git-scm.com/docs/gitattributes on * text=auto:

If you want to ensure that text files that any contributor introduces to the repository have their line endings normalized, you can set the text attribute to "auto" for all files.

@pdurbin
Copy link
Member

pdurbin commented Sep 22, 2023

@jp-tosca
Copy link
Contributor Author

@JR-1991 also went for just * text=auto at https://github.com/JR-1991/python-dvuploader/blob/main/.gitattributes

Hi Philip!

I was looking at this project, it seems to me this is not a dockized application. So, this means this wouldn't go through the unix > windows > unix (docker) cycle that the Dataverse would go on a windows machine. In case of applications that would go from unix > windows or windows > unix it makes sense to auto convert all files but when there is a step where the project is checked out on an operative system that is not the system that is running the conversion will cause the issues because it is not the target OS.

@pdurbin
Copy link
Member

pdurbin commented Oct 18, 2023

This pull request has merge conficts and unexpected changed files (should be just the addition of one file). As discussed at https://dataverse.zulipchat.com/#narrow/stream/375812-containers/topic/gitattributes.20files.20and.20windows.20auto.20conversion/near/397123361 I pushed two branches with the two proposed fixes:

@Sakshi-75 is on Windows and doing some testing for us. THANK YOU! ❤️

@pdurbin
Copy link
Member

pdurbin commented Nov 2, 2023

Merge conflicts and 33 files changed. Closing.

I'll leave an update in #9894 about next steps.

@pdurbin pdurbin closed this Nov 2, 2023
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

Successfully merging this pull request may close these issues.

Request for .gitattributes file to handle CRLF vs LF for SH Files
3 participants