search

swiftlang / swift-toolchain-sqlite

Copy of Sqlite for use by clients within the Swift toolchain. This is not a general-purpose wrapper for Sqlite.
Apache License 2.0
15 stars 2 forks source link

symbol prefixes? #3

Open weissi opened 1 month ago

weissi commented 1 month ago

I think we should prefix all the sqlite symbols from this. Otherwise if any other component uses a different sqlite we may get a clash on linker symbols.

In a previous live I've suggested this for another project.

FWIW, this is all it takes not to get symbol clashes which can have disastrous effects and whether they happen is actually undefined (happens on platform, linker arguments, potentially order of linker arguments, ...)

for f in *.c *.h; do gsed -ri 's/sqlite3_/swift_toolchain_sqlite3_/g' "$f"; done
compnerd commented 1 month ago

I'm not sure that's valuable currently. The locations we are using this currently are SPM and llbuild. If we build statically properly (with hidden visibility), the symbols are fully isolated and cannot escape.

weissi commented 1 month ago

I'm not sure that's valuable currently. The locations we are using this currently are SPM and llbuild. If we build statically properly (with hidden visibility), the symbols are fully isolated and cannot escape.

Yes, currently and If we build statically properly. Given the amount of configurations we have I'd almost bet that either doesn't quite hold today.

The workaround is a single shell command and we don't need to worry anymore, I think that's worth it :)

weissi commented 1 month ago

Another issue with packages like this is that given their prominent place (github.com/swiftlang), they'll be used as examples. So other people will either start using this or use this as an example of how to vendor SQLite. And currently this is done using the wrong or at least very dangerous approach.

compnerd commented 1 month ago

We currently use the system packages on the platforms where that is an issue, and transitioning to this package means that we have a clean slate to ensure that we do it properly :)

weissi commented 1 month ago

We currently use the system packages on the platforms where that is an issue, and transitioning to this package means that we have a clean slate to ensure that we do it properly :)

But you can't control all the builds and all the folks using this. And again: It's one one-off command away from namespacing it properly, I really don't see why you wouldn't do that.

Furthermore, symbol clashes aren't the only issue. It's also header clashes. Unless you @_implementationOnly import SwiftToolchainSQLite (which is unsupported), other clients of your modules will also see the SQLite header files you have in here. Those can also clash with potential ODR violations (although that's C++ arguably).

compnerd commented 1 month ago

https://github.com/swiftlang/swift-toolchain-sqlite/pull/4 is just as easy to ensure that it is built properly for everyone ;)

I am against it because it complicates work on the package:

> for f in *.c *.h; do gsed -ri 's/sqlite3_/swift_toolchain_sqlite3_/g' "$f"; done
At line:1 char:4
+ for f in *.c *.h; do gsed -ri 's/sqlite3_/swift_toolchain_sqlite3_/g' ...
+    ~
Missing opening '(' after keyword 'for'.
At line:1 char:21
+ for f in *.c *.h; do gsed -ri 's/sqlite3_/swift_toolchain_sqlite3_/g' ...
+                     ~
Missing statement body in do loop.
    + CategoryInfo          : ParserError: (:) [], ParentContainsErrorRecordException
    + FullyQualifiedErrorId : MissingOpenParenthesisAfterKeyword

Clearly that isn't just a simple thing. Doing this in the build system is easier, works everywhere, and doesn't require modifying the sources on each import.

compnerd commented 1 month ago

Changing the prefixes would also mean that it would be impossible for someone to use a different copy of SQLite (other than this package).

weissi commented 1 month ago

4 is just as easy to ensure that it is built properly for everyone ;)

I am against it because it complicates work on the package:

> for f in *.c *.h; do gsed -ri 's/sqlite3_/swift_toolchain_sqlite3_/g' "$f"; done
At line:1 char:4
+ for f in *.c *.h; do gsed -ri 's/sqlite3_/swift_toolchain_sqlite3_/g' ...
+    ~
Missing opening '(' after keyword 'for'.
At line:1 char:21
+ for f in *.c *.h; do gsed -ri 's/sqlite3_/swift_toolchain_sqlite3_/g' ...
+                     ~
Missing statement body in do loop.
    + CategoryInfo          : ParserError: (:) [], ParentContainsErrorRecordException
    + FullyQualifiedErrorId : MissingOpenParenthesisAfterKeyword

Clearly that isn't just a simple thing. Doing this in the build system is easier, works everywhere, and doesn't require modifying the sources on each import.

I think you're not using bash, try

bash -c 'for f in *.c *.h; do gsed -ri 's/sqlite3_/swift_toolchain_sqlite3_/g' "$f"; done'

or just sed if your sed is a GNU sed.

Regarding the header clashes, see https://github.com/swiftlang/swift/issues/53521

compnerd commented 1 month ago 
> bash -c 'for f in *.c *.h; do gsed -ri 's/sqlite3_/swift_toolchain_sqlite3_/g' "$f"; done'
bash : The term 'bash' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included, verify that the
path is correct and try again.
At line:1 char:1
+ bash -c 'for f in *.c *.h; do gsed -ri 's/sqlite3_/swift_toolchain_sq ...
+ ~~~~
    + CategoryInfo          : ObjectNotFound: (bash:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CommandNotFoundException
weissi commented 1 month ago 
> bash -c 'for f in *.c *.h; do gsed -ri 's/sqlite3_/swift_toolchain_sqlite3_/g' "$f"; done'
bash : The term 'bash' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included, verify that the
path is correct and try again.
At line:1 char:1
+ bash -c 'for f in *.c *.h; do gsed -ri 's/sqlite3_/swift_toolchain_sq ...
+ ~~~~
    + CategoryInfo          : ObjectNotFound: (bash:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CommandNotFoundException

WSL2 should have a bash, no? Alternatively, we can write the same thing in Swift :). I'm pretty sure you know what the intent is.

compnerd commented 1 month ago

I don't have WSL installed either :) Adding the whole build program, run program to import a new revision adds friction, friction which is easily avoided IMO. Would we be okay with adding a PowerShell/C# dependency for the package? (as a counter friction point for the macOS developers).

weissi commented 1 month ago

I don't have WSL installed either :) Adding the whole build program, run program to import a new revision adds friction, friction which is easily avoided IMO.

Would we be okay with adding a PowerShell/C# dependency for the package? (as a counter friction point for the macOS developers).

Fine, you can write it in Swift or in CMake or in Python or in anything else that's already a dependency.

Also note that you only need to do this to update SQLite. Currently there's no script provided to do that either, right?

compnerd commented 1 month ago

Correct, there is no script to update the package - that is a simple, extract and copy the source process. Building the SQLite amalgamation is pretty hefty process involving tcl, perl, etc.

weissi commented 1 month ago

Correct, there is no script to update the package - that is a simple, extract and copy the source process. Building the SQLite amalgamation is pretty hefty process involving tcl, perl, etc.

Yes, that's exactly my point. We should just tag the renaming at the end of the amalgamation process. Then we can also write it in TCL or Perl for extra fun

compnerd commented 1 month ago

Oh, if you want to do that upstream, that is a completely different point :). For this package, doing the very simple copy process is sufficient IMO. The amalgamation is performed by upstream at release, this just mirrors that into a git repo.