Fix os.path.relpath throwing #164
Conversation
|
Hello JornVernee, thanks for contributing a PR to our project! We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address jbvernee -(at)- xs4all -(dot)- nl. You can sign it at that link. If you think you've already signed it, please comment below and we'll check. |
|
I have already signed the OCA: https://openjdk.java.net/census#jvernee |
|
JornVernee has signed the Oracle Contributor Agreement (based on email address jbvernee -(at)- xs4all -(dot)- nl) so can contribute to this repository. |
|
Hey Doug, Just to make sure I'd like to wait for the old author to give the OK on me making a separate PR before merging this. It seems like they have abandoned their own PR, and don't really care, but they did come up with the fix after all. |
|
@dougxc Ok then, go ahead. |
|
Hi @JornVernee, thanks for taking a stab at this. In particular, there are cases where the logic simply doesn't work if a relative path can not be created, in those it needs to remain an error and the root case has to be fixed. In many cases it's OK anyway because relapth is used in conditions where a relative path can always be found (in particular when using In other cases, a more complex logic is needed. For example in intellij project generation, when a relative path can be found, the In other places a unix path is used as a short-cut to something else (a relative URL, a logical path in an archive etc.) and an other platform-independent approach should be used. In order to make progress, I propose that we don't fix everything at once but just fix the ones where |
|
@gilles-duboscq Yeah ok, maybe that was a bit optimistic of me (it was working after all, and together with some other changes I have I will revert the replacement and then try running a few commands to see which lines give the error. |
|
I see travis is running When doing that myself I get: Should that requirement be bumped up to 1.x.x ? P.S. Never mind, I think it's just using my python 3 Scripts folder. It doesn't look like my pylint version shown there actually works with python 2. P.P.S. Okay I installed 1.9.3 version of pylint for python 2, but running that with mx gate is throwing a lot of linter warnings, so I guess that is the reason the version hasn't been bumped up. Any idea where I can get the right version of pylint? |
…every path can be made relative on Windows, due to different drives. rename safe_relpath to maybe_relpath per suggestion Also rename callsites to maybe_relpath (oops) Revert replacements to do it again step-by-step to better examine use cases of os.path.relpath Fix regex to also work with windows path separators. Use maybe_relpath as a replacement for os.path.relpath a point where `mx ideinit` and `mx gate` are passing. Added special handling for PROJECTDIR relative paths. Added maybe_relpath to the other dependency branches as well linter fixes Removed duplicate '$PROJECT_DIR$/' replace other uses of PROJECT_DIR and relpath with safe equivalent. Consolidate code paths pash relpath_or_absolute calls into make_library since that is where they are joined with PROJECT_DIR (or not) using os.path.join instead of a hardcoded path separator. Revert "using os.path.join instead of a hardcoded path separator." since these are URIs and not actual paths Revert "Revert "using os.path.join instead of a hardcoded path separator." since these are URIs and not actual paths" Simplified things remove accidental diffs undiff more undiff more remove spurious withespace
JornVernee
force-pushed
the
safe_relpath
branch
from
c141d22 to
f22fbc0
Compare
|
I looked into this and decided to keep the changes simple, @gilles-duboscq or @dougxc can you take another look? I pushed the relpath lookup into make_library so it can either use I was also able to pass |
(You have to install strict versions and install astroid first with a strict version although it's an explicit dependency of pylint) |
Replaced calls to os.path.relpath with a safe alternative, since not every path can be made relative on Windows, due to different drives.
This is based on another PR [1], where the author asked someone else to take over since they don't have time to fix the merge conflicts [2].
I tried runningmx gatebut that throws an unrelated error, so it doesn't look like that works on Windows yet. So be aware that this has not passed gate yet.[1] : #134
[2] : #134 (comment)