You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by balidani <gi...@git.apache.org> on 2015/03/17 17:00:47 UTC
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
GitHub user balidani opened a pull request:
https://github.com/apache/flink/pull/491
Fix issue where Windows paths were not recognized as absolute
Hello!
An issue caused all tests to fail on Windows, because the path `file:/C:` was not recognized as absolute.
This pull-request fixes that issue.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/balidani/flink master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/491.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #491
----
commit d6768156a04e0f9fe3941ee0ec9d8300cfb0a0e3
Author: Dániel Bali <ba...@gmail.com>
Date: 2015-03-17T15:58:57Z
Fix issue where Windows paths were not recognized as absolute
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by balidani <gi...@git.apache.org>.
Github user balidani commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-87062926
So most of the jobs passed, except for PROFILE="-Dhadoop.version=2.6.0 -Dscala-2.11", where it says
No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-82584339
Yeah, that is a good start. Does it make sense to also check for the pattern "driverletter-colon" (c:) ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-86199581
I had a look at the Travis logs and it seems that the build failures are not related to your change.
We had a few issues with build stability on the master branch in the last days. Can you rebase your change to the current master and update the PR? That will trigger the build tests another time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-90507970
Btw, the path `file:/c:` that motivated this PR is not a valid absolute path according to Microsoft's documentation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by balidani <gi...@git.apache.org>.
Github user balidani commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-82625011
Hi!
I squashed them, I hope it's done correctly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-83453500
Before merging this, we should find out what are valid windows paths. The tests should be defined based on that, not based on what the `Path` class accepts ;-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by balidani <gi...@git.apache.org>.
Github user balidani commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-82586976
Yeah, that makes sense, I added two more assertions.
Sorry, I forgot to check checkstyle before committing.
Should I squash these commits in the end?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-93728544
This issue is discussed in FLINK-1848.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by balidani <gi...@git.apache.org>.
Github user balidani commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-93728981
Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by balidani <gi...@git.apache.org>.
Github user balidani commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-82567110
Hi!
I added some test cases to `PathTest`. Is it okay like this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by balidani <gi...@git.apache.org>.
Github user balidani commented on a diff in the pull request:
https://github.com/apache/flink/pull/491#discussion_r26613854
--- Diff: flink-core/src/test/java/org/apache/flink/core/fs/PathTest.java ---
@@ -63,6 +63,15 @@ public void testPathFromString() {
assertEquals("/my/path", p.toUri().getPath());
assertEquals("file", p.toUri().getScheme());
+ p = new Path("C:/my/windows/path");
+ assertEquals("/C:/my/windows/path", p.toUri().getPath());
+
+ p = new Path("file:/C:/my/windows/path");
+ assertEquals("/C:/my/windows/path", p.toUri().getPath());
+
+ p = new Path("C:");
+ assertEquals("/C:", p.toUri().getPath());
--- End diff --
Well the test passes, not sure if it's semantically correct though. If it's not correct, then `Path` should be changed I guess?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-89548376
I checked what are [valid absolute paths on windows](https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx#paths).
Summary:
A path is relative if it does not begin with any of the following:
- A disk designator **with a backslash**, for example `C:\` or `d:\`.
- A single backslash, for example, `\directory` or `\file.txt`. This is also referred to as an absolute path.
- A UNC name of any format, which always start with two backslash characters (`\\`).
**Note:** A path with disk designator but without backslash (such as `c:test.tmp`, or `D:tmp/test.tmp`) is relative.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/491#discussion_r26613687
--- Diff: flink-core/src/test/java/org/apache/flink/core/fs/PathTest.java ---
@@ -63,6 +63,15 @@ public void testPathFromString() {
assertEquals("/my/path", p.toUri().getPath());
assertEquals("file", p.toUri().getScheme());
+ p = new Path("C:/my/windows/path");
+ assertEquals("/C:/my/windows/path", p.toUri().getPath());
+
+ p = new Path("file:/C:/my/windows/path");
+ assertEquals("/C:/my/windows/path", p.toUri().getPath());
+
+ p = new Path("C:");
+ assertEquals("/C:", p.toUri().getPath());
--- End diff --
Is this a correct path in the end?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by balidani <gi...@git.apache.org>.
Github user balidani commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-86219748
Thanks, I did that, hopefully the tests will pass now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by balidani <gi...@git.apache.org>.
Github user balidani commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-83666214
I doubt that paths like `/C:/...` are valid on Windows. I guess Path should be changed then? How can we tell whether a path comes from Windows or not?
The Travis build failed. I looked at the logs, but I couldn't figure out why.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by balidani <gi...@git.apache.org>.
Github user balidani commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-82580762
I removed that part, I hope you meant it like this :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-82588549
Yeah, squashing would be good...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by balidani <gi...@git.apache.org>.
Github user balidani commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-93723421
Hi!
This PR was closed, but the commit right after mine removes the part that fixed the windows issue.
https://github.com/apache/flink/commit/23fe00629e4b36e5c4d3849c2711743148369631
As a results, tests still don't run on Windows. Could somebody take a look at this please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/491
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by balidani <gi...@git.apache.org>.
Github user balidani commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-82577438
I just realized that this fails in Travis, because in `hasWindowsDrive` (Path.java, line 282) there is `if (!OperatingSystem.isWindows()) {`. What can we do in this situation?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-82578379
I think that we should identify windows drives independently of whether we are on windows. The client may run on Linux, the cluster windows (or vice versa), that is totally valid.
I would change the method to not depend on the operating system.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: Fix issue where Windows paths were not recogni...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/491#issuecomment-82518745
Looks like a good fix. Could you add a test for this as well, to be sure that windows paths are recognized? Also a test that verifies that "isAbsolute()" still behaves properly with relative paths, after the fix?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---