Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Windows support #640
Windows support #640
Changes from all commits
73d6bd1
cea6670
a7b3338
d2d0f29
0dc444c
58fd321
48787eb
99e65ae
cb2dffb
407f55b
3f4b340
f334d31
1107907
7dd17f1
129b8b0
9e1ddef
eaf6d20
0bf528a
e610a5a
5b395c6
ea643d7
b908389
c3d1327
af302b8
362266c
71d015c
5913460
370bb8b
90b6650
2b05f8f
e0b9508
c09d03d
b4c687d
961c006
3232bbe
f0fae40
6f79d5d
0f75d48
526a556
a71f24d
0622125
0ad9f5a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #559 (comment) for context on redirects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see
sys.platform == "win32"
in many places. Should we have a constant that we create for this? Since much of our code only has a special path for Windows?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More I think about it I don't think this is necessary to have a constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a constant in Aaron's original code. Well, two constants, in different files. Since you need to share this between multiple files, you'd have to put it in
__init__
to avoid import issues. But we don't do this for any other platforms, so I decided to simply usesys.platform
, as everywhere else, to avoid introducing new entities.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet there is an interesting story here? Is this only something that showed up on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen this personally, but it's mentioned in one of the Aaron's commits:
Seems harmless, so I decided to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll admit using
os.path...
for urls is a hack and I should have used something like yarl for example. Thoughts? Since url paths are not the same as file paths? Maybe posixpath is the right thing to use here. It certainly seems better than os.path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate entity for URLs would be better, yes. But I'm not going to block this PR just because of this issue, since everything seems to work fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/conda-incubator/conda-store/pull/559/files#r1364084772 for context on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A fun bug 😄 glad we got to the bottom of this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to see the fallback is this approach 👍 If they are using cygwin or https://learn.microsoft.com/en-us/sysinternals/downloads/du it may still be possible they have du.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've already looked at du from sysinternals. Besides being a separate tool, it also produces slightly different results. And the output format is also different. As for cygwin, the context for this work is to be able to use native Windows tooling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 512? This is a weird size since it is a half a kb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment from a discussion on the original PR: "Note that the numbers will be off a little bit (by less than 512) on Mac because du rounds up to the nearest multiple of 512 when converting bytes to blocks."
I'm still not certain that macOS code is correct here, but it does pass this new test (where we check all three platforms), so I'm going to keep this and will take a look later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great glad this fix was minimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still not clear whether it means that Celery works as intended, since it's not officially supported on Windows. But it does run, so we'll just use this for now.