GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-85454: Remove distutils.ccompiler from Tools/c-analyzer by corona10 · Pull Request #95171 · python/cpython

Conversation

@corona10

@corona10 corona10 commented Jul 23, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Deprecate and remove distutils #85454

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10
pythongh-85454: Remove distutils.ccompiler from Tools/c-analyzer
d1f8cfa
@bedevere-bot bedevere-bot added the awaiting core review label Jul 23, 2022
@corona10 corona10 added the skip news label Jul 23, 2022
@corona10
corona10 requested a review from ericsnowcurrently July 23, 2022 14:02
corona10
corona10 commented Jul 23, 2022
View reviewed changes
Comment thread Tools/c-analyzer/c_parser/preprocessor/__init__.py
}


def _get_default_compiler():

@corona10 corona10 Jul 23, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Minimalized ported version from

cpython/Lib/distutils/ccompiler.py

Lines 937 to 956 in f4c0348

def get_default_compiler(osname=None, platform=None):
"""Determine the default compiler to use for the given platform.
osname should be one of the standard Python OS names (i.e. the
ones returned by os.name) and platform the common value
returned by sys.platform for the platform in question.
The default values are os.name and sys.platform in case the
parameters are not given.
"""
if osname is None:
osname = os.name
if platform is None:
platform = sys.platform
for pattern, compiler in _default_compilers:
if re.match(pattern, platform) is not None or \
re.match(pattern, osname) is not None:
return compiler
# Default to Unix compiler
return 'unix'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently Jul 25, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It may be worth having a short comment indicating this was derived from the distutils code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
kumaraditya303
kumaraditya303 reviewed Jul 23, 2022
View reviewed changes
Comment thread Tools/c-analyzer/c_parser/preprocessor/__init__.py


def _get_default_compiler():
if re.match('cygwin.*', sys.platform) is not None:

@kumaraditya303 kumaraditya303 Jul 23, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You can just remove cygwin as it is unsupported.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@corona10 corona10 Jul 23, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Are you sure?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@corona10 corona10 Jul 24, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We have a bunch of Cygwin-related codes (__CYGWIN__) in CPython code base, Did we decide to remove them?
Even if PEP 11 defined tier-based platforms, I don't interpret them as removing all unsupported platform codes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently Jul 25, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Cygwin is certainly still alive to some extent, so I'd say let's not worry about that in the PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@kumaraditya303 kumaraditya303 Jul 25, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If you remove cygwin branch, it will fallback to unix which is better than special casing it but it's minor and you can ignore this too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@corona10 corona10 Jul 26, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If you remove cygwin branch, it will fallback to unix which is better than special casing it but it's minor and you can ignore this too.

It's care about Windows/Cygwin case, see: https://docs.python.org/3/library/sys.html.
It can be gone to the wrong fallback logic.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ericsnowcurrently
ericsnowcurrently approved these changes Jul 25, 2022
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jul 25, 2022
@tiran
tiran merged commit a15ae19 into python:main Jul 25, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Jul 25, 2022
@corona10
corona10 deleted the gh-85454-c-analyzer branch July 25, 2022 23:38
@ericsnowcurrently ericsnowcurrently mentioned this pull request Aug 3, 2022
warnings.filters was modified by test_check_c_globals #95349
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes
@kumaraditya303 kumaraditya303 kumaraditya303 left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@corona10 @ericsnowcurrently @kumaraditya303 @tiran @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.