Skip to content

build: place executable content into the root of the build #515

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
Sep 3, 2019

Conversation

compnerd
Copy link
Member

Adjust the output location of the generated executable content to the root of
the build tree. This is needed primarily on Windows where there is no concept
of a RPATH, and the current directory is scanned for the dependent libraries.

@compnerd
Copy link
Member Author

@swift-ci please test

@@ -19,6 +19,12 @@ set(CMAKE_CXX_STANDARD 11)

set(CMAKE_C_VISIBILITY_PRESET hidden)

# NOTE(compnerd) this is a horrible workaround for Windows to ensure that the

Choose a reason for hiding this comment

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

Can you please do this for Windows only, and not for other platforms? I don't think having an identical directory layout between platforms is necessary; each have their own conventions which we should follow.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the same thing that LLVM does on all the platforms, as does the Swift project. What do we gain by doing it as a special case here? Furthermore, this is the build tree only, so having the identical directory layout is necessary to keep the overall packaging rules identical. (e.g. copying of files).

Choose a reason for hiding this comment

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

Ah, right, this is for the build layout, not the installed layout.

Adjust the output location of the generated executable content to the root of
the build tree.  This is needed primarily on Windows where there is no concept
of a RPATH, and the current directory is scanned for the dependent libraries.
@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/swift-package-manager#2329

@swift-ci please test

@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/swift-package-manager#2329
@swift-ci please test

@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/swift-package-manager#2329
apple/swift-corelibs-libdispatch#2498

@swift-ci please test

@compnerd
Copy link
Member Author

compnerd commented Sep 3, 2019

Please test with following PRs:
swiftlang/swift-package-manager#2329
swiftlang/swift-corelibs-foundation#2498

@swift-ci please test

1 similar comment
@compnerd
Copy link
Member Author

compnerd commented Sep 3, 2019

Please test with following PRs:
swiftlang/swift-package-manager#2329
swiftlang/swift-corelibs-foundation#2498

@swift-ci please test

@@ -19,6 +19,12 @@ set(CMAKE_CXX_STANDARD 11)

set(CMAKE_C_VISIBILITY_PRESET hidden)

# NOTE(compnerd) this is a horrible workaround for Windows to ensure that the

Choose a reason for hiding this comment

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

Ah, right, this is for the build layout, not the installed layout.

@compnerd
Copy link
Member Author

compnerd commented Sep 3, 2019

@ktopley-apple - okay to merge?

@ktopley-apple ktopley-apple merged commit 37c8c28 into swiftlang:master Sep 3, 2019
@ktopley-apple
Copy link
Contributor

Yes, done :-)

@compnerd compnerd deleted the output-directory branch September 3, 2019 18:27
@finagolfin
Copy link
Member

Unfortunately, @jakepetroules was right, as there are Swift tests that look for libdispatch.so and other files under the old build layout and are now disabled on the linux CI as a result of this pull, specifically all tests for the libdispatch and tsan_runtime features.

I confirmed by examining the build log for this commit and comparing with the previous commit's build log: note that libdispatch and tsan_runtime are missing from the available features for lit.py before the Swift testsuite is run only in the first linked log. This can be verified further through tests like Sanitizers/tsan or stdlib/DispatchData going from passing to unsupported after this pull.

I ran into this because I noticed that these tests were being run in prior Swift test logs I had run natively on Android, but were disabled recently, which I tracked down to this pull.

@finagolfin
Copy link
Member

finagolfin commented Sep 30, 2019

@compnerd or @drodriguez, should I file a bug in the Swift JIRA for this tsan/libdispatch issue with the linux CI, so that enabling those tests again isn't forgotten? Update: Filed.

yln pushed a commit to swiftlang/swift that referenced this pull request Oct 29, 2019
The swift-corelibs-libdispatch project had its build directory layout
changed [1] which silently deactivated the libdispatch tests on Linux,
because we use hard-coded paths to look for the required libdispatch
artifacts.  This change adapts the paths to the new layout.

Thanks to Jordan Rose for noticing and diagnosing this [2].
SR-11568

[2] https://bugs.swift.org/browse/SR-11568
[1] swiftlang/swift-corelibs-libdispatch#515
yln added a commit to swiftlang/swift that referenced this pull request Nov 12, 2019
* Reactivate libdispatch tests on Linux

The swift-corelibs-libdispatch project had its build directory layout
changed [1] which silently deactivated the libdispatch tests on Linux,
because we use hard-coded paths to look for the required libdispatch
artifacts.  This change adapts the paths to the new layout.

Thanks to Jordan Rose for noticing and diagnosing this [2].
SR-11568

[2] https://bugs.swift.org/browse/SR-11568
[1] swiftlang/swift-corelibs-libdispatch#515

* Fix libdispatch Swift module directory
rokhinip pushed a commit that referenced this pull request Nov 5, 2021
build: place executable content into the root of the build
Signed-off-by: Rokhini Prabhu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants