Skip to content

[CF] Unify implementations of _CFProcessPath. #2679

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

Merged
merged 1 commit into from
Aug 24, 2020

Conversation

3405691582
Copy link
Member

@3405691582 3405691582 commented Feb 19, 2020

Firstly, _CFProcessPath has no implementation for TARGET_OS_BSD. Indeed,
there is no portable implementation of this action across multiple
platforms. As an initial implementation for TARGET_OS_BSD, use the
environment to help us. Some shells set the environment variable "_" to
the path used to invoke the binary. This mirrors the implementation for
TARGET_OS_OSX, so add the same conditionals on __CFProcessIsRestricted.

Relying on the shell has drawbacks, including that the shell doesn't set
the full path to the current executable if it isn't part of the PATH
(or, of course, specified explicitly). Perhaps on some other platforms
there is some miscellaneous kernel trickery that could be done, but
let's defer that to the future and keep getenv("_") as a last resort.

Second, given that, we should bring the implementations together (like
we've done elsewhere), not to mention that _CFProcessPath is fundamental
to other parts of CF. This highlights the repetition in each
implementation, so here we also factor off some of it and share it
across the implementations to facilitate code reuse.

@3405691582 3405691582 force-pushed the CFPlatform_CFProcessPath branch from 319ca8c to f1d8f03 Compare February 19, 2020 00:16
@3405691582
Copy link
Member Author

@compnerd This is mainly a mechanical change, but could you double check this on the win32 buildbot please?

@compnerd
Copy link
Member

@millenomi
Copy link
Contributor

@swift-ci please test

@3405691582
Copy link
Member Author

macOS test failure looks unrelated, since it seems to be in xctest and that it has been failing in the same fashion since at least build 2072 on March 30.

@compnerd
Copy link
Member

@3405691582 this needs to be rebased as there is now a conflict.

@3405691582 3405691582 force-pushed the CFPlatform_CFProcessPath branch from f1d8f03 to 0502fb4 Compare May 31, 2020 21:08
Firstly, _CFProcessPath has no implementation for TARGET_OS_BSD. Indeed,
there is no portable implementation of this action across multiple
platforms. As an initial implementation for TARGET_OS_BSD, use the
environment to help us. Some shells set the environment variable "_" to
the path used to invoke the binary. This mirrors the implementation for
TARGET_OS_OSX, so add the same conditionals on __CFProcessIsRestricted.

Relying on the shell has drawbacks, including that the shell doesn't set
the full path to the current executable if it isn't part of the PATH
(or, of course, specified explicitly). Perhaps on some other platforms
there is some miscellaneous kernel trickery that could be done, but
let's defer that to the future and keep getenv("_") as a last resort.

Second, given that, we should bring the implementations together (like
we've done elsewhere), not to mention that _CFProcessPath is fundamental
to other parts of CF. This highlights the repetition in each
implementation, so here we also factor off some of it and share it
across the implementations to facilitate code reuse.
@3405691582 3405691582 force-pushed the CFPlatform_CFProcessPath branch from 0502fb4 to 59410fe Compare May 31, 2020 21:21
@3405691582
Copy link
Member Author

Rebase completed.

@spevans
Copy link
Contributor

spevans commented Jun 5, 2020

@swift-ci please test

1 similar comment
@spevans
Copy link
Contributor

spevans commented Jun 18, 2020

@swift-ci please test

@millenomi millenomi merged commit 68f9ab3 into swiftlang:master Aug 24, 2020
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.

4 participants