Skip to content

test: avoid os-dependant path #18952

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 1 commit into from
May 16, 2020
Merged

test: avoid os-dependant path #18952

maflcko merged 1 commit into from
May 16, 2020

Conversation

@fametrano
Copy link
Contributor

@fametrano fametrano commented May 11, 2020 •
edited by maflcko
Loading

The current code fails on windows because of the forward slashes; using os.path.join solves the problem and it is in general more robust

@fametrano
avoided os-dependant path
8a22fd0 
The current code fails on windows because of the forward slashes; using os.path.join solves the problem and it is in general more robust
@fametrano fametrano changed the title avoided os-dependant path avoid os-dependant path May 11, 2020
@DrahtBot DrahtBot added the Tests label May 11, 2020
@fanquake
Copy link
Member

fanquake commented May 13, 2020

@sipsorcery want to take a look here & confirm the fix for windows?

@sipsorcery
Copy link
Contributor

sipsorcery commented May 13, 2020

Hmm Windows should typically be able to deal with / paths. Maybe it's a Python on Windows problem. I'll verify.

@maflcko
Copy link
Member

maflcko commented May 13, 2020

Jup, I don't think this is a bug fix. We've been running the functional tests on windows just fine.

@elichai
Copy link
Contributor

elichai commented May 13, 2020

FWIW, In the future if we really want to be have the slashes agnostic, we can use pathlib.Path which overrides the Div operator to insert slashes so you could do: Path(some_path) / 'src' / 'bitcoind'.

https://docs.python.org/3/library/pathlib.html

@laanwj
Copy link
Member

laanwj commented May 13, 2020

I like using os.path.join here (it's consistent—what we use in other places too). But yes, please change the motivation if it's a refactor not a fix.

@sipsorcery
Copy link
Contributor

sipsorcery commented May 13, 2020

I can verify that the current test_framework.py on master doesn't have any path issues when run on Windows.

It's hard to see where it could ever become a problem since modern day Windows accepts / as an alternative path separator.

The command below works fine on my Windows 10 machine:

cd c:/dev\github/sipsorcery_bitcoin//build_msvc

@fametrano
Copy link
Contributor Author

fametrano commented May 16, 2020

I import the test suite code in another project to automate regtest testing: when looking for the bitcoind binary on a Windows 10 machine, Python 3.8.2, no WSL, the original code fails.
Also, just two lines below the code is using os.path.join, so consistency is lacking

@maflcko maflcko changed the title avoid os-dependant path test: avoid os-dependant path May 16, 2020
@maflcko
Copy link
Member

maflcko commented May 16, 2020

ACK 8a22fd0

@maflcko maflcko merged commit f8123d4 into bitcoin:master May 16, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 17, 2020
Merge bitcoin#18952: test: avoid os-dependant path
dc52784 
8a22fd0 avoided os-dependant path (Ferdinando M. Ametrano)

Pull request description:

  The current code fails on windows because of the forward slashes; using os.path.join solves the problem and it is in general more robust

ACKs for top commit:
  MarcoFalke:
    ACK 8a22fd0

Tree-SHA512: 813f27aea33f97c8afac52e716a55fc5d7fb69621023aba99d40df7e1d145e0ec8d1eee49ddd403b219bf0e0e168e0e987b05c78eaef611f744d99bf2fc8bc91
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 1, 2021
@fametrano @PiRK
avoided os-dependant path
d733ebb 
Summary:
using os.path.join is in general more robust than using string with forward slashes

This is a backport of Core [[bitcoin/bitcoin#18952 | PR18952]]

Test Plan: `ninja check-functional`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D9119
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@fametrano @fanquake @sipsorcery @maflcko @elichai @laanwj @DrahtBot