Skip to content

Fixes #112 install command doesn't use platform in nt_user scheme#113

Merged
jaraco merged 7 commits intopypa:mainfrom
zooba:issue112
Jan 30, 2022
Merged

Fixes #112 install command doesn't use platform in nt_user scheme#113
jaraco merged 7 commits intopypa:mainfrom
zooba:issue112

Conversation

@zooba
Copy link
Copy Markdown
Contributor

@zooba zooba commented Jan 24, 2022

Fixes #112

@zooba
Copy link
Copy Markdown
Contributor Author

zooba commented Jan 24, 2022

Still needs a specific test, and there are issues with the existing test that need fixing, so please do not merge until I've done those

@zooba
Copy link
Copy Markdown
Contributor Author

zooba commented Jan 24, 2022

I think this is ready to commit. Bit weird that distutils.sysconfig and sysconfig disagree[d?] on the include directory, but it seems to be handled okay.

'xx',
)
else:
expect_headers = os.path.join(sysconfig.get_python_inc(0, ''), 'xx')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm struggling to understand the intention behind divergent tests here. It's annoying that the tests need to have such intimate knowledge of the implementation details in order to set expectations. Is there a better way that derives this expectation from the API?

Is it possible the expected value could be derived from some value in sysconfig (not distutils.sysconfig)? I'm trying to move more of the logic away from distutils.sysconfig as that module is deprecated, so this line moves in the wrong direction.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's because Windows doesn't include the runtime version in its headers subdirectory ({prefix}\Include\xx), while POSIX does ({prefix}\include\python3.10\xx).

But POSIX also seems to include the ABI flags in the name? Sometimes? I'm sure the logic is "fine", but I couldn't figure it out. Pretty sure it's also changed upstream with rebasing onto sysconfig, though not in any way that impacts default builds after 3.7.

If you weren't running 3.7 tests on this repo, I wouldn't even have noticed the change and would've stuck with my simpler test logic :)

@jaraco jaraco merged commit b53a824 into pypa:main Jan 30, 2022
@zooba zooba deleted the issue112 branch January 31, 2022 17:29
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.

install command doesn't use platform in user site directory on Windows

2 participants