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

linux: remove swiftImageInspectionShared #14877

Closed
wants to merge 1 commit into from

Conversation

compnerd
Copy link
Member

Merge this into the runtime as expected rather than having a separate
component.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

@compnerd
Copy link
Member Author

compnerd commented Feb 28, 2018

CC: @spevans @bob-wilson @jrose-apple @jckarter

What could go wrong?

@compnerd
Copy link
Member Author

@swift-ci please test Linux platform

@bob-wilson
Copy link
Contributor

I think I have a better solution. At least I understand better what happened to cause this. Let me create a PR....

@bob-wilson
Copy link
Contributor

See #14880

@bob-wilson
Copy link
Contributor

(I'm not opposed to removing the library if that's the right thing to do, but I'd much prefer a more minimal solution for Swift 4.1.)

@compnerd
Copy link
Member Author

@swift-ci please test macOS platform

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 7feea226230a51d247dc41652b509912b7bdd97a

@swift-ci
Copy link
Contributor

swift-ci commented Mar 1, 2018

Build failed
Swift Test OS X Platform
Git Sha - 7feea226230a51d247dc41652b509912b7bdd97a

@compnerd
Copy link
Member Author

compnerd commented Mar 1, 2018

@swift-ci please test macOS platform

@bob-wilson
Copy link
Contributor

Even if this is the right thing to do, can you please hold off merging it until after we've tested #14880 and confirmed that it fixes SR-7038?

@compnerd
Copy link
Member Author

compnerd commented Mar 1, 2018

@bob-wilson sure thing

@compnerd
Copy link
Member Author

@bob-wilson okay with me rebasing/merging this?

@bob-wilson
Copy link
Contributor

Now that we've sorted out SR-7038, I have no objections from a process perspective. On the technical front, I am confused. When we tried to stop linking that library, things broke, right? What is different now?

@compnerd
Copy link
Member Author

The change is merging the provider into the runtime itself. So when the runtime is linked, the dependency is satisfied.

@bob-wilson
Copy link
Contributor

OK, I guess that should be fine then. Someone else who understands this better should review it.

@jrose-apple
Copy link
Contributor

That's probably @jckarter (sorry, @jckarter).

@jrose-apple jrose-apple requested a review from jckarter March 14, 2018 15:46
@jckarter
Copy link
Contributor

jckarter commented Mar 14, 2018

I haven't been following these changes close enough to comment, sorry. If @compnerd and @spevans think it's OK, they're probably in the best position to say so.

@CodaFi
Copy link
Contributor

CodaFi commented Nov 18, 2019

@compnerd Could you rebase this so we can get it merged?

@compnerd compnerd force-pushed the static-is-not-shared branch from 7feea22 to f65d7f4 Compare January 12, 2020 20:30
@compnerd
Copy link
Member Author

CC: @spevans

@CodaFi necromancing level infinity! :-p

@compnerd
Copy link
Member Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - f65d7f4d1e6f038fa7b4abc5196e06dcc183e9de

Merge this into the runtime as expected rather than having a separate
component.
@compnerd compnerd force-pushed the static-is-not-shared branch from f65d7f4 to fdf1c35 Compare January 12, 2020 21:19
@compnerd
Copy link
Member Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - fdf1c35

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - fdf1c35

@compnerd
Copy link
Member Author

@swift-ci please clean test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - fdf1c35

@compnerd
Copy link
Member Author

@swift-ci please test macOS platform

@CodaFi
Copy link
Contributor

CodaFi commented Jan 13, 2020

Cool!

@spevans
Copy link
Contributor

spevans commented Jan 13, 2020

The reason for have ImageInspectionShared and ImageInspectionStatic is that there are two versions of lookupSymbol that need to be linked in:

The version in ImageInspectionShared uses dladdr() and is used to support -static-stdlib . The other version in ImageInspectionStatic uses its own implementaion to parse a static ELF binary when used with -static-executable. This is used by #29039

@compnerd
Copy link
Member Author

I think that it makes sense to remove lookupSymbol from SwiftRT-ELF.cpp. That file is meant for the metadata introspection. It would make sense for that code-movement to be part of #29039 I think.

@shahmishal
Copy link
Member

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

  • How to change the base branch: (Link)
  • More detail about the branch update: (Link)

@spevans
Copy link
Contributor

spevans commented Oct 5, 2020

@compnerd I have done an updated version of this PR in #34180

@compnerd
Copy link
Member Author

compnerd commented Oct 5, 2020

Great, I'll close this off then.

@compnerd compnerd closed this Oct 5, 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.

8 participants