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

subversion: use sqlite from MacOS #76970

Closed
wants to merge 1 commit into from

Conversation

markphip
Copy link
Contributor

This fixes a problem when using the JavaHL library from Eclipse where
the system version of the sqlite dependency is loaded instead of the
Homebrew version. Building and using the system sqlite resolves the
problem.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

I do not feel great about this change since subversion has used the sqlite dependency from Homebrew "forever". I have not been able to figure out the root cause for this problem. I first noticed it when upgrading an Intel Mac to Big Sur so I think it is related to that, but ISTR Homebrew 3.0 happened around the same time.

I suspect this problem only happens from Eclipse but cannot be sure. Big Sur includes a reasonably recent version of sqlite so I do not really mind using that version, but I cannot speak to Mojave and Catalina.

Finally .. this is the error as reported by JavaHL:

svn: Couldn't perform atomic initialization
SQLite error
svn: SQLite compiled for 3.35.5, but running with 3.32.3

This fixes a problem when using the JavaHL library from Eclipse where
the system version of the sqlite dependency is loaded instead of the
Homebrew version. Building and using the system sqlite resolves the
problem.
@BrewTestBot BrewTestBot added java Java use is a significant feature of the PR or issue perl Perl use is a significant feature of the PR or issue ruby Ruby use is a significant feature of the PR or issue labels May 10, 2021
@Bo98
Copy link
Member

Bo98 commented May 10, 2021

Big Sur includes a reasonably recent version of sqlite so I do not really mind using that version, but I cannot speak to Mojave and Catalina.

Yosemite: 3.8.5
El Capitan: 3.8.10.2
Sierra: 3.14.0
High Sierra: 3.19.3
Mojave: 3.24.0
Catalina: 3.28.0
Big Sur: 3.32.3

Looks like subversion requires 3.8.2 or later so this should work for everything.

@markphip
Copy link
Contributor Author

I have confirmed with this change the library works correctly and the problem is gone. That aside ... in terms of trying to diagnose the root cause. The installed library without this change looks fine to me:

otool -L /Library/Java/Extensions/libsvnjavahl-1.dylib
/Library/Java/Extensions/libsvnjavahl-1.dylib:
	/opt/homebrew/opt/subversion/lib/libsvnjavahl-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 905.6.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_repos-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_client-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_wc-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_ra-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_diff-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_ra_local-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_fs-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_ra_svn-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libsasl2.2.dylib (compatibility version 3.0.0, current version 3.15.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_ra_serf-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/libexec/serf/lib/libserf-1.dylib (compatibility version 1.3.0, current version 1.3.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_delta-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_subr-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_fs_fs-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_fs_x-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/Cellar/subversion/1.14.1_2/lib/libsvn_fs_util-1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libexpat.1.dylib (compatibility version 7.0.0, current version 8.0.0)
	/usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.11)
	/opt/homebrew/opt/sqlite/lib/libsqlite3.0.dylib (compatibility version 9.0.0, current version 9.6.0)
	/opt/homebrew/opt/lz4/lib/liblz4.1.dylib (compatibility version 1.0.0, current version 1.9.3)
	/opt/homebrew/opt/utf8proc/lib/libutf8proc.2.dylib (compatibility version 2.0.0, current version 2.4.1)
	/opt/homebrew/opt/apr-util/libexec/lib/libaprutil-1.0.dylib (compatibility version 7.0.0, current version 7.1.0)
	/opt/homebrew/opt/apr/libexec/lib/libapr-1.0.dylib (compatibility version 8.0.0, current version 8.0.0)
	/opt/homebrew/opt/gettext/lib/libintl.8.dylib (compatibility version 11.0.0, current version 11.0.0)
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1775.118.101)
	/System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 59754.100.106)
	/System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices (compatibility version 1.0.0, current version 1122.33.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.100.5)

If I sample Eclipse from Activity Monitor I can see that it has loaded the system version of the sqlite library before I have even used SVN. When I do something that causes the SVN JavaHL load, it looks like it tries to load the Homebrew sqlite but the system one still is used? Maybe this is a sqlite thing where two versions of the library cannot be loaded?

       0x13ac3c000 -        0x13ac4bfff +libsvn_ra-1.0.dylib (0) <4B19FAAF-D2B3-372E-A334-40737CFFF8C3> /opt/homebrew/*/libsvn_ra-1.0.dylib
       0x13afe0000 -        0x13afebfff +libsvn_ra_local-1.0.dylib (0) <80D99B4B-AEA0-365A-89B6-64D01AE6E1E6> /opt/homebrew/*/libsvn_ra_local-1.0.dylib
       0x13fab4000 -        0x13fb13fff +libsvnjavahl-1.0.dylib (0) <05EC8E60-54DE-3008-B1FA-70894D0758A4> /opt/homebrew/*/libsvnjavahl-1.0.dylib
       0x13fb64000 -        0x13fb93ff7 +libsvn_repos-1.0.dylib (0) <A2C359BA-1A2E-316D-AC6D-28CE43D06FD7> /opt/homebrew/*/libsvn_repos-1.0.dylib
       0x13fbb0000 -        0x13fbc3ffb +libsvn_diff-1.0.dylib (0) <D24EE8DF-AD26-33E0-A061-C31583F272A9> /opt/homebrew/*/libsvn_diff-1.0.dylib
       0x13fbd8000 -        0x13fbe3fff +libsvn_fs-1.0.dylib (0) <8AEC19C8-6FD3-3349-B26F-9665FF4F9114> /opt/homebrew/*/libsvn_fs-1.0.dylib
       0x13fe9c000 -        0x13ff03fff +libsvn_client-1.0.dylib (0) <B6E8D012-D3C1-3C6E-97C8-7B8BA9699220> /opt/homebrew/*/libsvn_client-1.0.dylib
       0x13ff28000 -        0x13ff3ffff +libsvn_ra_svn-1.0.dylib (0) <7104EA7A-2353-38E7-8FFD-7B1630938997> /opt/homebrew/*/libsvn_ra_svn-1.0.dylib
       0x13ff58000 -        0x13ff77ffb +libsvn_ra_serf-1.0.dylib (0) <6927157A-C7AF-39D6-B394-170E19AB3EA9> /opt/homebrew/*/libsvn_ra_serf-1.0.dylib
       0x13ff94000 -        0x13ff9bffb +libsvn_fs_util-1.0.dylib (0) <9611A4A4-FCFE-3BA5-9619-28D9817AAD5D> /opt/homebrew/*/libsvn_fs_util-1.0.dylib
       0x157e98000 -        0x157f17ff3 +libsvn_wc-1.0.dylib (0) <6B07A708-B38D-374A-A860-A1D8881530F2> /opt/homebrew/*/libsvn_wc-1.0.dylib
       0x157f40000 -        0x157f53fff +libserf-1.1.3.0.dylib (0) <DA629BAA-7517-3E7D-A178-6FAF64276341> /opt/homebrew/*/libserf-1.1.3.0.dylib
       0x157f70000 -        0x157f87fff +libsvn_delta-1.0.dylib (0) <8A993A77-1CB1-3AFF-B94C-993FFF8661E4> /opt/homebrew/*/libsvn_delta-1.0.dylib
       0x157fa0000 -        0x157fdbfff +libsvn_fs_fs-1.0.dylib (0) <F91ACCD4-590C-39B5-AC18-7E69D9D1E1E8> /opt/homebrew/*/libsvn_fs_fs-1.0.dylib
       0x168dec000 -        0x168e43ffb +libsvn_subr-1.0.dylib (0) <BA28A248-8B7D-3667-8E6A-EDABB148E44F> /opt/homebrew/*/libsvn_subr-1.0.dylib
       0x168e74000 -        0x168eafffb +libsvn_fs_x-1.0.dylib (0) <AFB1C36D-C7C6-3522-A793-C5177E0C62CB> /opt/homebrew/*/libsvn_fs_x-1.0.dylib
       0x168ed0000 -        0x168eebffb +liblz4.1.dylib (0) <242C39CC-17CB-3C9F-8297-178F64969CD2> /opt/homebrew/*/liblz4.1.dylib
       0x168efc000 -        0x168f17ff7 +libaprutil-1.0.dylib (0) <14E5462B-6CD9-3230-8CE5-6BBC4CFD8B60> /opt/homebrew/*/libaprutil-1.0.dylib
       0x16ee08000 -        0x16eeebff7 +libsqlite3.0.dylib (0) <73D77613-00CB-364D-97BB-3DAE840B6E45> /opt/homebrew/*/libsqlite3.0.dylib
...snip...
       0x185af3000 -        0x185b05ff7  libprotobuf-lite.dylib (4146) <EC0554F4-F99C-39A5-99A0-ECCB101EF423> /System/Library/PrivateFrameworks/WirelessDiagnostics.framework/Versions/A/Libraries/libprotobuf-lite.dylib
       0x185b06000 -        0x185b4d7ff  com.apple.awd (1.0 - 951) <0C89EB83-85A7-32D2-85D2-60D63C258955> /System/Library/PrivateFrameworks/WirelessDiagnostics.framework/Versions/A/WirelessDiagnostics
       0x185b4e000 -        0x185cbda5b  com.apple.Montreal (1.0 - 142) <CE167A1C-9A53-3099-A93C-E714332416C6> /System/Library/PrivateFrameworks/Montreal.framework/Versions/A/Montreal
       0x185cbe000 -        0x185da93eb  com.apple.NLP (1.0 - 210.2) <B6F8469F-C1EB-3370-B2A5-A6255741BBA8> /System/Library/PrivateFrameworks/NLP.framework/Versions/A/NLP
       0x185daa000 -        0x1861528af  com.apple.CoreData (120 - 1048) <BF206551-0234-3C90-B455-20CCBD84294F> /System/Library/Frameworks/CoreData.framework/Versions/A/CoreData
       0x186153000 -        0x186169e9b  com.apple.ProtocolBuffer (1 - 285.24.10.20.1) <3C5E7738-35A5-33FF-B2D4-BAB92317A9CA> /System/Library/PrivateFrameworks/ProtocolBuffer.framework/Versions/A/ProtocolBuffer
       0x18616a000 -        0x186307fff  libsqlite3.dylib (321.3) <D8C91B63-0FA2-3208-9386-630B6A4C893F> /usr/lib/libsqlite3.dylib

So maybe the root issue is that something in Eclipse is using CoreData and that prevents using another sqlite from the same process?

A workaround I have used that worked was to launch Eclipse from a terminal after first running export DYLD_LIBRARY_PATH=/opt/homebrew/opt/sqlite/lib. This is not a realistic solution for most users to do though and I believe SIP makes it impossible to set this globally so that just launching the app would pickup the change.

@markphip
Copy link
Contributor Author

I suspect also that the issue you mention in #76900 (comment) could be a bug in Eclipse, as it sounds like it is assuming the default library search path, but shouldn't be.

@carlocab Eclipse is a java app. The Subclipse plugin uses JNI to load the JavaHL library that library then loads its dependencies using the OS. JNI does not provide any mechanism to control how the library will load its dependencies. On Windows, we are able to use a trick of the OS to use JNI to preload all of the dependencies in reverse order. This allows us to use JNI to find the library and then as each library loads and tries to load its dependency since it is already loaded in memory it is able to use that version. This trick does not work on Linux or MacOS though and we have to rely on how the OS loads dependencies.

I suspect the issue might be that you just cannot load multiple versions of sqlite in the same process. Maybe Eclipse recently started using CoreData somewhere?

@markphip
Copy link
Contributor Author

Big Sur includes a reasonably recent version of sqlite so I do not really mind using that version, but I cannot speak to Mojave and Catalina.

Yosemite: 3.8.5
El Capitan: 3.8.10.2
Sierra: 3.14.0
High Sierra: 3.19.3
Mojave: 3.24.0
Catalina: 3.28.0
Big Sur: 3.32.3

Looks like subversion requires 3.8.2 or later so this should work for everything.

@Bo98 my main concern would be DB compatibility. Imagine a svn command line user checks something out with current version. So they have a sqlite 3.35.5 database created in their working copy. Now we change the CLI to use the system sqlite. Will that older version be able to read the database that was originally created with a newer version?

On Big Sur (3.32.3) the answer is yes from what I can tell. Maybe that is true for all of these versions (thanks for providing them by the way). I am just not sure if that is the case or not.

@carlocab
Copy link
Member

The Subclipse plugin uses JNI to load the JavaHL library that library then loads its dependencies using the OS. JNI does not provide any mechanism to control how the library will load its dependencies.

Seems to me that JavaHL should know how to find its own libraries. Either that, or the order it searches paths is wrong.

@markphip
Copy link
Contributor Author

Seems to me that JavaHL should know how to find its own libraries. Either that, or the order it searches paths is wrong.

That is what I am attempting to show in the output from otool and the activity monitor sample. The best I can tell the JavaHL library is correct and it does know how to load the dependencies. That is why I am suspect it is something specific about sqlite itself and the fact that the system version of the library has already been loaded by some other Eclipse plugin. I am just guessing though.

@carlocab
Copy link
Member

carlocab commented May 10, 2021

Have you looked at the output from starting Eclipse with DYLD_PRINT_LIBRARIES=1 set? ...though this probably shows equivalent information to the snippet you've pasted above.

@Bo98
Copy link
Member

Bo98 commented May 10, 2021

Maybe this is a sqlite thing where two versions of the library cannot be loaded?

Yeah, in general loading two versions of a library ends up to bad things so I'm not surprised. dyld probably tries to prevent this.

Indeed, CoreData uses system sqlite so anything linking to CoreData should by connection also link to system sqlite, which is where the mismatch comes in if linking to brewed subversion with its brewed sqlite.

@markphip
Copy link
Contributor Author

Have you looked at the output from starting Eclipse with DYLD_PRINT_LIBRARIES=1 set? ...though this probably shows equivalent information to the snippet you've pasted above.

I was not aware of this option, it will be handy in the future, but yes it just shows the same information I got from Activity Monitor. I can see that JavaHL loads the correct Homebrew dependencies, I guess the one loaded by CoreData just takes precedence.

@carlocab
Copy link
Member

I guess the one loaded by CoreData just takes precedence.

In this case, I think using system sqlite is the correct move here (barring unforeseen complications).

@markphip
Copy link
Contributor Author

@Bo98 my main concern would be DB compatibility. Imagine a svn command line user checks something out with current version. So they have a sqlite 3.35.5 database created in their working copy. Now we change the CLI to use the system sqlite. Will that older version be able to read the database that was originally created with a newer version?

I have been doing some research and from what I can tell this should not be a problem as the sqlite file format should be both backwards and forwards compatible.

I think we should try to make this change.

@markphip
Copy link
Contributor Author

I realize this could be a controversial change .. what more needs to be discussed to make a decision on approving or closing it?

@markphip
Copy link
Contributor Author

bump ... is there anything more I can do for this to help?

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

OK, let's give this a shot.

(I hope you don't mind if I ping you for any issues that arise from this, @markphip. But I don't think there should be any.)

@markphip
Copy link
Contributor Author

I do not mind being pinged, and I tried to see if anyone in the Subversion community had concerns and did not hear anything.

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@cho-m cho-m mentioned this pull request May 22, 2021
5 tasks
@gromgit gromgit mentioned this pull request Jun 20, 2021
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 21, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
java Java use is a significant feature of the PR or issue outdated PR was locked due to age perl Perl use is a significant feature of the PR or issue ruby Ruby use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants