WIP: add command to check dynamic library linkage #4765
Conversation
|
Whoa, awesome! There is definitely interest in having something like this. |
|
I think this is a great idea! Pushed a commit to rename it. Glad to hear there's interest! I'll beef up the tests (thanks for the suggestion), and add support for |
|
Looks like the one failure on Travis is unrelated to my PR? It succeeded on the other jobs. |
|
@mistydemeo I restarted that build and it failed again. I'm happy to take a look at that Ruby/RubyGems combination locally if you'd like? |
|
Looks like the test is flaky; my |
This new command, doctor, checks for common problems. Currently, it looks for broken dynamic library links in C extensions. It scans all of the specifications in the bundle for .bundle files in C extensions. If any of the dynamic libraries linked against no longer exist, bundler will report a message to the console.
This will eventually support other tests, so the dylib test needs to be able to to meaningfully return an empty result
|
Rebased on master, and made some extra changes:
Any other changes you'd like to see? |
|
Hooray! 🎉 🏆 I'm super excited about this, thank you so much! @homu r+ |
|
📌 Commit 8fa722c has been approved by |
WIP: add command to check dynamic library linkage This new command, linkage, checks for broken dynamic library links in C extensions. I'd heard there was some interest in adding this functionality, so I thought I'd submit a preliminary PR for discussion. In my experience, broken dylib linkage is a common issue with C extensions, so I think having a good way to diagnose it would be valuable. This command scans all of the specifications in the bundle for .bundle files in C extensions. If any of the dynamic libraries linked against no longer exist, bundler will report a message to the console and exit non-0. TODOs: * Add support for non-Darwin OSs * Improve tests A few questions: * Is there a good way to mock functionality in the tests? Doing it in the standard rspec way isn't working since `bundle :command` runs in a subprocess. I'd like to be able to stub out stuff that actually checks dylibs and the like. * Is making this a new command the right approach? I assumed this wouldn't be ideal to include in, say, `check` because it would slow it down.
| Otherwise, Bundler prints a success message and exits with a status of 0. | ||
|
|
||
| The bundle's gem dependencies must all be installed to run this command; if | ||
| they are not, Bundler prints an error message and exits with a status of 2. |
There was a problem hiding this comment.
This is no longer correct, I should update these docs. Will pushing a new commit mess with the approval you just granted?
There was a problem hiding this comment.
I think that I have just cancelled the imminent merge of this PR. Go ahead and push another commit, and I'll kick off another build to get a final merge with green tests. Thanks for following up!
|
@homu r- |
| end | ||
|
|
||
| def otool_available? | ||
| system("otool --version 2>&1 >/dev/null") |
There was a problem hiding this comment.
we can't use /dev/null and need to use Bundler::NULL instead because of Windows
There was a problem hiding this comment.
Sure thing, though this line is only run on Darwin. Same with ldd_available? below.
|
Updated. |
|
This only failed on one job, and it seems unrelated to my PR. |
|
Sometimes the suite is kind of flaky on old Rubies. :/ hopefully it will pass for merging! @homu r+ |
|
📌 Commit 6117de9 has been approved by |
WIP: add command to check dynamic library linkage This new command, linkage, checks for broken dynamic library links in C extensions. I'd heard there was some interest in adding this functionality, so I thought I'd submit a preliminary PR for discussion. In my experience, broken dylib linkage is a common issue with C extensions, so I think having a good way to diagnose it would be valuable. This command scans all of the specifications in the bundle for .bundle files in C extensions. If any of the dynamic libraries linked against no longer exist, bundler will report a message to the console and exit non-0. TODOs: * Add support for non-Darwin OSs * Improve tests A few questions: * Is there a good way to mock functionality in the tests? Doing it in the standard rspec way isn't working since `bundle :command` runs in a subprocess. I'd like to be able to stub out stuff that actually checks dylibs and the like. * Is making this a new command the right approach? I assumed this wouldn't be ideal to include in, say, `check` because it would slow it down.
|
💔 Test failed - status |
|
☀️ Test successful - status |
This new command, linkage, checks for broken dynamic library links in C extensions.
I'd heard there was some interest in adding this functionality, so I thought I'd submit a preliminary PR for discussion. In my experience, broken dylib linkage is a common issue with C extensions, so I think having a good way to diagnose it would be valuable.
This command scans all of the specifications in the bundle for .bundle files in C extensions. If any of the dynamic libraries linked against no longer exist, bundler will report a message to the console and exit non-0.
TODOs:
A few questions:
bundle :commandruns in a subprocess. I'd like to be able to stub out stuff that actually checks dylibs and the like.checkbecause it would slow it down.