Skip to content

linux: remove swiftImageInspectionShared #14877

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

linux: remove swiftImageInspectionShared #14877

wants to merge 1 commit into from

Conversation

@compnerd
Copy link
Member

@compnerd compnerd commented Feb 28, 2018

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 •
edited
Loading

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

What could go wrong?
@compnerd
Copy link
Member Author

compnerd commented Feb 28, 2018

@swift-ci please test Linux platform
@bob-wilson
Copy link
Contributor

bob-wilson commented Feb 28, 2018

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

bob-wilson commented Feb 28, 2018

See #14880
@bob-wilson
Copy link
Contributor

bob-wilson commented Feb 28, 2018

(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

compnerd commented Feb 28, 2018

@swift-ci please test macOS platform
@swift-ci
Copy link
Contributor

swift-ci commented Feb 28, 2018

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

bob-wilson commented Mar 1, 2018

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

compnerd commented Mar 12, 2018

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

bob-wilson commented Mar 13, 2018

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

compnerd commented Mar 13, 2018

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

bob-wilson commented Mar 14, 2018

OK, I guess that should be fine then. Someone else who understands this better should review it.
@jrose-apple
Copy link
Contributor

jrose-apple commented Mar 14, 2018

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 •
edited
Loading

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

compnerd commented Jan 12, 2020

CC: @spevans

@CodaFi necromancing level infinity! :-p
@compnerd
Copy link
Member Author

compnerd commented Jan 12, 2020

@swift-ci please test
@swift-ci
Copy link
Contributor

swift-ci commented Jan 12, 2020

Build failed
Swift Test Linux Platform
Git Sha - f65d7f4d1e6f038fa7b4abc5196e06dcc183e9de
@compnerd
linux: remove swiftImageInspectionShared
fdf1c35 
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

compnerd commented Jan 12, 2020

@swift-ci please test
@swift-ci
Copy link
Contributor

swift-ci commented Jan 12, 2020

Build failed
Swift Test OS X Platform
Git Sha - fdf1c35
@swift-ci
Copy link
Contributor

swift-ci commented Jan 12, 2020

Build failed
Swift Test Linux Platform
Git Sha - fdf1c35
@compnerd
Copy link
Member Author

compnerd commented Jan 12, 2020

@swift-ci please clean test
@swift-ci
Copy link
Contributor

swift-ci commented Jan 13, 2020

Build failed
Swift Test OS X Platform
Git Sha - fdf1c35
@compnerd
Copy link
Member Author

compnerd commented Jan 13, 2020

@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

compnerd commented Jan 13, 2020

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

shahmishal commented Oct 1, 2020

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

8 participants

@compnerd @bob-wilson @swift-ci @jrose-apple @jckarter @CodaFi @spevans @shahmishal