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

Win fixes #3409

Merged
merged 1 commit into from Sep 30, 2014
Merged

Win fixes #3409

merged 1 commit into from Sep 30, 2014

Conversation

jbmohler
Copy link
Contributor

two windows related tweaks

The doc build issue makes clutters 'git status' output after building docs because windows git doesn't handle the sym links correctly (win 7; git 1.9.4). I think its a decent solution in a bad situation.

@tacaswell
Copy link
Member

👎 on messing with the symlinks unless it is wrapped in win-specific cut-outs.

@jbmohler
Copy link
Contributor Author

I added the conditionals (squashing the commits) and a warning since changing git-controlled files is just too wrong to pass silently. At least now the warning might aid understanding and will give more documentation in make.py of the ugly.

shutil.copytree(os.path.join(link, '..', target), link)

if sys.platform == 'win32' and len(symlink_warnings) > 0:
print('The following items related to symlinks will show up '+ \
Copy link
Member

Choose a reason for hiding this comment

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

There is a preference for () line continuation over \

git on windows really says 'unsupported symlink foo removed' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

git on windows checks on the symlink as a one line text file with the target as the content of said file (why git doesn't properly use the machinery behind mklink is a whole other issue -- NTFS does support symlinks although no one uses them). The shutil.copytree & requisite prior delete will cause spurious changes in git status. The actual string "'unsupported symlink {} removed'" is in the code I just wrote.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I tend to be overly terse (particularly before I have had coffee).

I think it would be best if the error message matched (as closely as possible) what git status would print out if run in the top level directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, that's a good idea; done

@tacaswell
Copy link
Member

Thanks this looks better to me.

@tacaswell
Copy link
Member

@Tillsten as another know windows developer can you take a look at this?

@tacaswell
Copy link
Member

This may be out-of-scope for this PR, but would it be possible to replace the symlink files after building the docs is done?

@jbmohler
Copy link
Contributor Author

I considered replacing them but it still imposes risk of confusion if the doc-making process dies. It's just plum ugly and I'd rather not write more code on ugly.

@tacaswell
Copy link
Member

Fair enough.

@tacaswell
Copy link
Member

lgtm to me @mdboom can you take a look at this as well?

@tacaswell tacaswell modified the milestones: v1.4.1, v1.4.x Sep 3, 2014
for link, target in required_symlinks:
if sys.platform == 'win32' and os.path.isfile(link):
Copy link
Member

Choose a reason for hiding this comment

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

stupid question, does 64bit windows id as win32?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the check is consistent with many other checks in MPL setup.py related code & verified on python3.4 64 bit myself

@tacaswell
Copy link
Member

@Tillsten @cgohlke Could you have a look at this as other windows devs?

@Tillsten
Copy link
Contributor

Tillsten commented Sep 6, 2014

I never was able to compile mpl on windows, so i am not sure i am the right person to ask. Maybe i'll give it another try tomorrow to be able to give some feedback.

@tacaswell
Copy link
Member

@Tillsten This is just about building the docs (which will use what ever version of mpl it can import).

@tacaswell tacaswell modified the milestones: v1.4.x, v1.4.1 Sep 8, 2014
@jbmohler jbmohler mentioned this pull request Sep 12, 2014
tacaswell added a commit that referenced this pull request Sep 30, 2014
BLD/DOC : patch over git + symlink issues on windows
@tacaswell tacaswell merged commit 9dbf4eb into matplotlib:v1.4.x Sep 30, 2014
@tacaswell
Copy link
Member

@jbmohler Thanks for this, sorry it languished for so long.

@jbmohler jbmohler deleted the win_fixes branch October 20, 2014 10:57
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.

None yet

4 participants