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

Fix EOL formatting on Windows #407

Merged
merged 2 commits into from
Jan 6, 2017
Merged

Conversation

jakirkham
Copy link
Member

This configures git to normalize more line endings to avoid differences caused by a user's development environment (e.g. Windows vs. Unix). By doing this, we can be sure to get LF in all the cases we were already accustomed to having them. With Windows specific files (e.g. Batch files), we use CRLF as Windows would expect. This extends it beyond simply bld.bat as was the case before.

@jakirkham jakirkham changed the title Extend git's EOL configuration to all files in the feedstock WIP: Extend git's EOL configuration to all files in the feedstock Jan 3, 2017
@jakirkham
Copy link
Member Author

To make this easier to test @nicoddemus , I went ahead and merged this with PR ( #401 ). Will back that out once we are done testing. Please let me know if this stops conversion of everything to CRLF.

@nicoddemus
Copy link
Member

Thanks @jakirkham.

Re-rendered py-feedstock with your branch, here's the commit.

It is still changing the line endings of the local files though:

λ git st
On branch rerender-1.7.1.dev9
Your branch is up-to-date with 'origin/master'.
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

        new file:   .gitattributes
        modified:   .gitignore
        modified:   .travis.yml
        modified:   LICENSE
        modified:   README.md
        modified:   appveyor.yml
        modified:   ci_support/run_docker_build.sh
        modified:   circle.yml

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   .gitignore
        modified:   .travis.yml
        modified:   LICENSE
        modified:   README.md
        modified:   appveyor.yml
        modified:   circle.yml

Looking at my local files, I see their line endings are still CRLF. I think the way conda-smithy is rendering the files is messing with Git's autocrlf setting, which in my case is the default (true).

How does conda-smithy write the text of the files to disk? It seems that just reading/writing \n characters and using normal strings writes files using the native line endings:

λ git status
On branch master
Your branch is up-to-date with 'origin/master'.
nothing to commit, working tree clean

λ e:\Miniconda3\python.exe
Python 3.5.2 |Continuum Analytics, Inc.| (default, Jul  5 2016, 11:41:13) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> n = '.travis.yml'
>>> open(n).readlines()[0]
'# This file was generated automatically from conda-smithy. To update this configuration,\n'
>>> open(n, 'rb').readlines()[0]
b'# This file was generated automatically from conda-smithy. To update this configuration,\r\n'
>>>
>>> lines = open(n).readlines()
>>> open(n, 'w').writelines(lines)
>>> open(n, 'rb').readlines()[0]
b'# This file was generated automatically from conda-smithy. To update this configuration,\r\n'
>>> ^Z

λ git status
On branch master
Your branch is up-to-date with 'origin/master'.
nothing to commit, working tree clean

Update: took a cursory glance at the code, it seems it does the write thing on the write_file function... perhaps the problem is the files which are copied directly like the license and .gitattributes files?

@jakirkham
Copy link
Member Author

The local files still have CRLF, but does the cache? In any event, I agree with you that this is still not a fix. Though we may be making some progress if the cache is at least correct.

@nicoddemus
Copy link
Member

The local files still have CRLF, but does the cache?

I'm pretty sure it does because I remember just doing a git commit -m "..." (without an -a) for 6537d11 but I can confirm this tomorrow.

@jakirkham
Copy link
Member Author

Yeah, now that I think about it, you are probably right. I'll take a closer look at this tomorrow.

@jakirkham
Copy link
Member Author

So I don't see lots of calls to readlines, writelines, or splitlines for that matter. Though we do use io.open to write files. I have tweaked our calls to io.open to force the use of LF-style newlines. Please let me know if that helps.

@jakirkham jakirkham changed the title WIP: Extend git's EOL configuration to all files in the feedstock WIP: Fix EOL formatting on Windows Jan 4, 2017
@nicoddemus
Copy link
Member

Rá, bingo!

nicoddemus/py-feedstock@9db93cc

And my local copy, after creating a fresh branch off origin/master:

λ e:\Miniconda3\Scripts\conda-smithy.exe rerender
No circle token.  Create a token at https://circleci.com/account/api and
put it in ~/.conda-smithy/circle.token
No appveyor token. Create a token at https://ci.appveyor.com/api-token and
Put one in ~/.conda-smithy/appveyor.token
No anaconda token. Create a token via
  anaconda auth --create --name conda-smithy --scopes "repos conda api"
and put it in ~/.conda-smithy/anaconda.token
Fetching package metadata ...........
Fetching package metadata ...........
Fetching package metadata ...........
Fetching package metadata ...........
Fetching package metadata ...........

Re-rendered with conda-smithy 1.7.0.post.dev13.

You can commit the changes with:

    git commit -m 'MNT: Re-rendered with conda-smithy 1.7.0.post.dev13'

These changes need to be pushed to github!

λ git st
On branch rerender-1.7.0.post.dev13
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

        new file:   .gitattributes
        modified:   .travis.yml
        modified:   README.md
        modified:   appveyor.yml
        modified:   ci_support/run_docker_build.sh

That seems to do it! 🎉

The last tweak, the recommended message should use " on Windows instead of ':

    git commit -m "MNT: Re-rendered with conda-smithy 1.7.0.post.dev13"

Thanks a lot @jakirkham! 😁

@jakirkham
Copy link
Member Author

The last tweak, the recommended message should use " on Windows instead of ':

   git commit -m "MNT: Re-rendered with conda-smithy 1.7.0.post.dev13"

Sounds reasonable. Would you be willing to do a PR? 😉

@jakirkham
Copy link
Member Author

Glad to see it is working. One question though. In your example, it seems to have the .gitattributes changes that use to be in this PR, but I thought I had backed out. Have they crept back in somehow?

@jakirkham
Copy link
Member Author

In any event, glad we seem to have figured out what was going on and fixed it. Will try to investigate this odd test failure now.

@nicoddemus
Copy link
Member

Sounds reasonable. Would you be willing to do a PR?

Sure, will do it right away. 😁

In your example, it seems to have the .gitattributes changes that use to be in this PR, but I thought I had backed out.

Oops that was my bad. I merged with my local changes (which contained 2178f18) to your fork at 3d804b3, instead of resetting to it.

After I reset conda-smithy to 3d804b3 the rendering is as we expect it: nicoddemus/py-feedstock@05d0a6c.

@nicoddemus
Copy link
Member

Opened a PR at #408

@jakirkham
Copy link
Member Author

After I reset conda-smithy to 3d804b3 the rendering is as we expect it: nicoddemus/py-feedstock@05d0a6c.

Phew. 😅 Good to know that was not needed.

@jakirkham
Copy link
Member Author

So this was passing yesterday with similar changes. The main difference appears to be conda-build 2.1.0 vs. 2.0.12. So the import failure is burying something else. Will start digging.

for line in fh_src:
fh_dst.write(line)

shutil.copymode(src, dst)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I had added this to ensure all files that we copy from conda-smithy have the right line endings. However, I expect we probably don't need this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be needed as demonstrated by the comment below. Otherwise files that have been copied end up with the wrong EOL formatting.

@jakirkham
Copy link
Member Author

Thanks so much for your help testing this @nicoddemus . If it isn't too much to ask, would you mind doing one more test?

I had some code that forced line endings of copied files that I wanted to try removing. If this still works ok without it, I'd like to not include it.

@jakirkham jakirkham changed the title WIP: Fix EOL formatting on Windows Fix EOL formatting on Windows Jan 6, 2017
@nicoddemus
Copy link
Member

If it isn't too much to ask, would you mind doing one more test?

Not at all, glad I can help!

I had some code that forced line endings of copied files that I wanted to try removing. If this still works ok without it, I'd like to not include it.

Rerendering with the latest commit in this branch unfortunately changes the line endings of these files: .gitattributes, LICENSE, checkout_merge_commit.sh and .gitignore.

@jakirkham
Copy link
Member Author

Thanks @nicoddemus . That is very helpful. I have added back the commit that reads and then writes each file to the destination to copy them. So this should be back to working order.

@nicoddemus
Copy link
Member

Thanks @jakirkham, here's the rerender for the last commit in this branch: nicoddemus/py-feedstock@f0e598e

So I think this is good to go. 👍

@jakirkham
Copy link
Member Author

jakirkham commented Jan 6, 2017

Great timing. Just came to give this one more look over. I'll back out the merge of the channel config PR ( #401 ) since this seems to be working. Thanks again for all of your help debugging and testing.

Edit: For reference the commit being tested above was ( 069cfea ) (no longer in this PR).

@jakirkham
Copy link
Member Author

Also note to future self and future others, this assumes the LF is the only EOL character that should be used and it should be used everywhere. This works now as everything does use LF currently. However, that may not be the case in the future if say batch files were included in the feedstock files (not the recipe). This should be an easy fix for writing files as we can expose the newline argument. Though it will be a bit trickier for copying the feedstock_content as we don't have a good way to configure the newline on a per file basis. Given this will take more work/thought to handle that case and we have no need of it at present, it makes more sense to leave addressing that case for the future when it is actually needed.

@jakirkham jakirkham merged commit c33de2f into conda-forge:master Jan 6, 2017
@jakirkham jakirkham deleted the set_ci_eol branch January 6, 2017 16:30
@jakirkham
Copy link
Member Author

This has been included into the config channels PR ( #401 ) explicitly.

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.

2 participants