Skip to content
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

[DO NOT MERGE] remove reference to swiftImageInspectionShared static library #14860

Closed
wants to merge 1 commit into from

Conversation

bob-wilson
Copy link

Even the name of this library shows that it is not a static library....
The static-stdlib-args.lnk file is generated by the
gen-static-stdlib-link-args script. The previous change for SR-7038
(PR 14772) only fixed the reference in the static-executable-args.lnk file.
rdar://problem/37710244

Even the name of this library shows that it is not a static library....
The static-stdlib-args.lnk file is generated by the
gen-static-stdlib-link-args script. The previous change for SR-7038
(PR 14772) only fixed the reference in the static-executable-args.lnk file.
rdar://problem/37710244
@bob-wilson
Copy link
Author

@swift-ci please test and merge

@jckarter
Copy link
Member

Can we add a regression test that attempts to link (and if executable_test is available, maybe execute) a statically-linked binary on Linux, so that this is less likely to regress again?

@jrose-apple
Copy link
Contributor

It looks like that's what Driver/static-stdlib-linux.swift is, since it just failed. My next question is how it passed before.

Copy link
Collaborator

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I'm not sure how this is supposed to work. The swiftImageInspectionShared is what contains the runtime and the rest of the image inspection parts. If this is being removed, which I'm fine with conceptually, we need to ensure that we do not remove ImageInspectionELF.cpp from the runtime sources.

@jrose-apple
Copy link
Contributor

The static standard library is supposed to be completely static. It doesn't get to link against .sos. Or am I missing something?

@bob-wilson bob-wilson changed the title utils: remove reference to swiftImageInspectionShared static library [DO NOT MERGE] remove reference to swiftImageInspectionShared static library Feb 28, 2018
@compnerd
Copy link
Collaborator

compnerd commented Feb 28, 2018

@jrose-apple okay, so that is what I would assume too. However, there was linkage to the shared inspection component. The way that we currently are handling the static build seems pretty complicated overall. Contrary to what you would think, the shared image inspection is used ONLY for static builds.

@jrose-apple
Copy link
Contributor

Who wrote this originally? Was it @jckarter?

@bob-wilson
Copy link
Author

I was just looking at this. The "Shared" library is actually built as a static library:

add_swift_library(swiftImageInspectionShared STATIC ....)

@jrose-apple
Copy link
Contributor

/me throws up hands, quits guessing about the problem without looking

@bob-wilson
Copy link
Author

I just downloaded the latest dev toolchain for Linux and there is no copy of that library installed. That would explain why we're getting link failures. This might just be a CMake problem.

@compnerd
Copy link
Collaborator

@jrose-apple @spevans wrote that originally

@compnerd
Copy link
Collaborator

I don't know the reason for the complexity, lets see what breaks on PR#14877. Perhaps we have evolved enough to avoid a lot of the complexity that necessitated that back when it was introduced?

@spevans
Copy link
Collaborator

spevans commented Feb 28, 2018

I think the static linking needs to be reworked as I think it broke when the ELF section changes were reworked. The problem with adding a regression test for it was that it also relied on building a fully static libICU (the one that ships with ubuntu needs -ldl) so a test couldnt be added to reliably test it.
So removing the current broken reference is fine for now.

@jckarter
Copy link
Member

jckarter commented Feb 28, 2018

It used to be that we needed a different archive for the image inspection part of things because a static binary doesn't have access to libdl, which is where some of the complexity arose. With @compnerd's changes we use a static constructor, which should work uniformly with or without a dynamic linker available.

@bob-wilson
Copy link
Author

Ross and I looked at a build log from one of the Linux bots. The swiftImageInspectionShared library is not even being built.

@bob-wilson
Copy link
Author

See #14880. I'm going to close this PR

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.

None yet

5 participants