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

Use startswith('linux') to check for the Linux platform #1263

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

sschuberth
Copy link
Collaborator

@sschuberth sschuberth commented Nov 6, 2018

This is to support #1262

@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

Merging #1263 into develop will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1263      +/-   ##
===========================================
- Coverage    83.29%   83.29%   -0.01%     
===========================================
  Files          117      117              
  Lines        12922    12922              
===========================================
- Hits         10764    10763       -1     
- Misses        2158     2159       +1
Impacted Files Coverage Δ
src/scancode/interrupt.py 39.47% <0%> (-2.64%) ⬇️
src/scancode/api.py 93.33% <0%> (-1.49%) ⬇️
src/typecode/pygments_lexers.py 52.25% <0%> (-0.65%) ⬇️
src/scancode/cli.py 77.39% <0%> (+0.65%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86722b3...3c55c59. Read the comment docs.

@pombredanne
Copy link
Member

@sschuberth thank you. Do you mind adding you sign-off (either in each of the commit or at least in the body of this PR)?

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!. Please focus only on Linux and let Windows and macOS aside for now.

etc/conf/base.py Outdated
os = 'linux'
elif 'win32' in sys_platform:
elif sys_platform == 'win32':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be some risk attached to changing the tests here... Do you mind leaving these as is?

etc/configure.py Outdated
platform_names = ('posix', 'linux',)
elif 'win32' in sys_platform:
elif sys_platform == 'win32':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above... please only change the Linux test.

@@ -1052,10 +1052,10 @@ def copy_required_modules(dst_prefix, symlink):
if f is not None:
f.close()
# special-case custom readline.so on OS X, but not for pypy:
if modname == 'readline' and sys.platform == 'darwin' and not (
if modname == 'readline' and is_darwin and not (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not change anything in virtualenv... this is the official/standard code so it should not be modified .

@pombredanne pombredanne changed the title Use startswith() to check the system platform again 'linux' Use startswith('linux') to check Linux system platform Nov 6, 2018
@sschuberth sschuberth force-pushed the sys-platform-startswith branch from f47d9b6 to 46d3573 Compare November 6, 2018 20:28
@sschuberth
Copy link
Collaborator Author

@pombredanne, I've addressed all your comments.

Some platforms like Alpine Linux actually return 'linux2' here, i.e. they
include the major Linux version as a suffix, also see [1].

[1] https://docs.python.org/dev/library/sys.html#sys.platform

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth force-pushed the sys-platform-startswith branch from 46d3573 to 3c55c59 Compare November 6, 2018 20:29
@sschuberth sschuberth changed the title Use startswith('linux') to check Linux system platform Use startswith('linux') to check for the Linux platform Nov 6, 2018
@pombredanne
Copy link
Member

@sschuberth Thanks++, merging

@pombredanne pombredanne merged commit 812e0f4 into develop Nov 6, 2018
@pombredanne pombredanne deleted the sys-platform-startswith branch November 6, 2018 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants