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 path comparison between tools/bazel and bazelisk source #98

Merged
merged 1 commit into from Nov 6, 2019

Conversation

@clintharrison
Copy link
Contributor

clintharrison commented Oct 30, 2019

The os.path.join() here was resulting in ~/foo/./tools/bazel being compared to ~/foo/tools/bazel, which should actually match, but didn't.

@philwo
philwo approved these changes Oct 31, 2019
Copy link
Member

philwo left a comment

Thank you!

@philwo

This comment has been minimized.

Copy link
Member

philwo commented Oct 31, 2019

@clintharrison Alternatively, what about using os.path.samefile here?

Return True if both pathname arguments refer to the same file or directory. This is determined by the device number and i-node number and raises an exception if an os.stat() call on either pathname fails.

@clintharrison clintharrison force-pushed the clintharrison:master branch 2 times, most recently from c4941a0 to bf995c7 Oct 31, 2019
@clintharrison

This comment has been minimized.

Copy link
Contributor Author

clintharrison commented Oct 31, 2019

@clintharrison Alternatively, what about using os.path.samefile here?

Return True if both pathname arguments refer to the same file or directory. This is determined by the device number and i-node number and raises an exception if an os.stat() call on either pathname fails.

Cool! I like that more -- I didn't realize py3 supported that on Windows now. It's not Windows-compatible in py2, but it looks like we don't intend on having py2 compatibility anymore?

@philwo

This comment has been minimized.

Copy link
Member

philwo commented Oct 31, 2019

Mhm, I would like to preserve Python 2.7 compatibility until the end of this year. What about this:

try:
  if not os.path.samefile(wrapper, __file__):
    return wrapper
except AttributeError:
  # Python 2.x on Windows does not support os.path.samefile yet.
  if os.path.abspath(wrapper) != os.path.abspath(__file__):
    return wrapper

We can then remove the try/catch when we finally drop Python 2.x support together with everyone else in the world beginning of 2020 😊

@clintharrison clintharrison force-pushed the clintharrison:master branch from bf995c7 to 8540d6d Nov 5, 2019
@clintharrison

This comment has been minimized.

Copy link
Contributor Author

clintharrison commented Nov 5, 2019

Ok, that's fair! That looks like a good alternative to me.

(Though I do wonder if anyone runs it with py2 in practice? It has a py3 shebang.)

@philwo
philwo approved these changes Nov 6, 2019
@philwo

This comment has been minimized.

Copy link
Member

philwo commented Nov 6, 2019

Ok, that's fair! That looks like a good alternative to me.

(Though I do wonder if anyone runs it with py2 in practice? It has a py3 shebang.)

You have a point there :D I just checked the issue history and found that I actually promised users in #6 (comment) that it's compatible with Python 2 and 3. I don't remember why I didn't change the shebang to just /usr/bin/python at the time. 🤔

However, luckily this will all be much simpler in 2 months, when we can move to Python 3 only.

@philwo philwo merged commit 3df9b19 into bazelbuild:master Nov 6, 2019
2 checks passed
2 checks passed
buildkite/bazelisk Build #323 passed (1 minute, 39 seconds)
Details
cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.