Skip to content

Use -print-target-info instead of a hardcoded triple #2883

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 25, 2020

Conversation

neonichu
Copy link
Contributor

This has been present since the initial commit of SwiftPM and may have been needed back then, but isn't necessary anymore today. This brings the added bonus of compiling the manifest for the host architecture instead of a hardcoded x86_64.

@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

@jakepetroules jakepetroules left a comment

Choose a reason for hiding this comment

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

I don't think we should do this, because we lose the deployment target consistency. That means if I'm running on macOS 11 and you're running on macOS 10.15, you might fail to compile the manifest.

Can we instead just read the default target from the compiler and swap the version for 10.15, and then use that?

@compnerd
Copy link
Member

@jakepetroules - Im not sure I understand what you mean by the deployment target consistency? The compiler will default to the build system which is where the manifest is going to be executed. The minimum deployment target will implicitly match the target system here.

@neonichu
Copy link
Contributor Author

neonichu commented Aug 21, 2020

Oh you mean because I could be using APIs which aren't available on an older OS? I guess that's a good point.

@jakepetroules
Copy link
Contributor

Oh you mean because I could be using APIs which aren't available on an older OS? I guess that's a good point.

That's right. Deployment target is something intrinsic to the code being compiled, not something that can be determined automatically.

@neonichu
Copy link
Contributor Author

Yep, I wasn't really thinking about the fact that developers can use arbitrary APIs in the manifest.

@abertelrud
Copy link
Contributor

Is there a way to keep the deployment target as it is without also having to specify the architecture (leaving it to the compiler to select the host, so SwiftPM doesn't need to reimplement that logic)? At some point one might want to build for a different architecture than what the host supports, but for the moment, it seems better to specify the deployment target and leave out the architecture.

@neonichu
Copy link
Contributor Author

Yah, that would be nice, but I don't think there's a way to do so. Open for any suggestions. Right now I'm planning to update this to essentially use -print-target-info.

@neonichu neonichu force-pushed the manifest-does-not-need-target branch 2 times, most recently from 1caf966 to 9fd1884 Compare August 24, 2020 23:36
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu neonichu changed the title Remove -target flag from manifest loading Use -print-target-info instead of a hardcoded triple Aug 24, 2020
@neonichu neonichu force-pushed the manifest-does-not-need-target branch from 9fd1884 to fbf1b04 Compare August 24, 2020 23:45
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu neonichu force-pushed the manifest-does-not-need-target branch from fbf1b04 to ee97d75 Compare August 25, 2020 17:29
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

/Users/buildnode/jenkins/workspace/swift-package-manager-PR-osx-smoke-test/branch-master/swiftpm: error: manifest parse error(s):
11:03:12 :0: error: Swift requires a minimum deployment target of OS X 10.9

Looks like something is going wrong in the bootstrapped SwiftPM, we're probably constructing the wrong -target.

@neonichu neonichu force-pushed the manifest-does-not-need-target branch from ee97d75 to 9c8b37c Compare August 25, 2020 18:30
@neonichu
Copy link
Contributor Author

Ah yes, the triple string was constructed completely wrong 🤦‍♂️

@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

neonichu commented Aug 25, 2020

Linux failure doesn't look related:

12:21:54 make[1]: Leaving directory '/home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-master/build/buildbot_incremental/libicu-linux-x86_64/data'
12:21:54 Makefile:153: recipe for target 'install-recursive' failed
12:21:54 make: *** [install-recursive] Error 2```

@neonichu
Copy link
Contributor Author

@swift-ci please smoke test linux

@neonichu neonichu merged commit 8ee8745 into swiftlang:master Aug 25, 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