-
Notifications
You must be signed in to change notification settings - Fork 188
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
Remove MiniPortile from the extension build #345
Conversation
@metaskills here's the PR I was talking about in the issue yesterday evening. Sorry in for the code-bomb, moving MiniPortile out of the extension build was not quite as simple as I had originally presumed. Who would have guessed? 🙄 I've not yet tested this on development hosts running on OSX or Windows as my dev machine is running Ubuntu. If you happen to have either I'd very much welcome a native test on one of those architectures. |
Thanks, I'll dig into the PR this evening. |
Amazing! Great work! I have a few questions which could be a few small changes. Let's talk first before doing any ebb/flow work. TravisCI & CircleCI Ubuntu TestingCan Travis/Circle continue to use the test/bin install scripts? If I am reading this all correctly, by not installing the OpenSSL/FreeTDS in extconsts.rb, means we are using whatever is on the system. Is this #4 you mentioned? Thoughts? How do we test the version of FreeTDS listed in extconsts? Doing a Rake Build (simulated gem install)This failed for me. I am on Darwin and use MacPorts to install everything in /opt. The following command did work for me tho. $ bundle exec rake build -- \
--with-freetds-include=/opt/local/include/freetds \
--with-freetds-lib=/opt/local/lib This would be true if I was doing a gem install too right? If so, should we search a few standard dirs or help a bit with this? I compile all my rubies with The rake ports:cross TaskI am running this now and love it! Works like a champ. Cant wait to get to 2.0.0.pre1 out. |
Glad you like it! Regarding Travis/Circle I believe both builds run If it's not using the ports installed libraries on Travis and Circle that is not the intent and I can fix it. :) Regarding the Rake Build Yes you are correct, it would be true for a normal build. There was a LOT of logic for searching alternate load paths in the build which I removed, largely because I could not understand what the correct load-order should be given all the alternatives. If we start checking Presuming the build breaks, if we decide not to modify anything then the fix for the gem's install would be to use bundle config similar to: bundle config build.tiny_tds \
--with-freetds-include=/opt/local/include/freetds \
--with-freetds-lib=/opt/local/lib This will tell bundler to use those flags every time it needs to install the "tiny_tds" gem. If that's not satisfactory then I'm willing to revisit automatically adding those load paths, but I'll need help testing it since I'm not running OSX. Either way I should update the documentation to document the existence of the |
So I just did some testing and I think you're right, the CircleCI and Travis builds will defer to using the system This is, very similar to the point 4 that I made. I would personally prefer system libraries be used over ports libraries on systems which have them (I have a theory that this might solve the slow load time on OpenSSL 1.0.2 on Windows). However, as you mention tests should definitely reference the provided libraries otherwise they grant false confidence to builds. :) When I next get some hack time I'll fix this as well. |
@metaskills I have set aside some hack time this weekend, so if you have any preferences about the above please let me know. Specifically how do you feel about the different solutions to the OSX load path? Is I'll also be fixing the test libraries not being loaded in the various automated test services. |
@coderjoe Whoop! Sorry for the late reply.
Me too... but I would still want to explicitly install dependencies that we use for the Windows static builds on non-Windows envs to ensure those versions work. Simply adding the
Ugh... I do feel this is a bit sticky... but would supporting the
Yes please! Though I thought we had some. |
I am sorry but I don't understand what you mean. Are you saying that you want to make sure that the correct sources are downloaded and installed based on what is in Or are you trying to say you'd rather explicitly install the libraries via shell scripts in the CI builds to make CI explicitly test as though something was installed by a user? That's also very possible and would be relatively simple to switch back to. Just let me know which you meant or if I'm still off base. :)
/opt is not Darwin specific, but
We already had it yes, but in my opinion it added complexity for almost no gain. Personally I'd prefer not to re-implement it since I don't like any software overriding my user, distribution, or OS specific library load orders. (More on this below.)
I don't agree that explicitly overriding the library search and include paths in the tiny_tds gem build in order to support MacPorts and The short version is that if we add it back then by default the gem will override user, distribution, and OS specific library search paths. On my development machine, this will cause If users have custom use cases (MacPorts, Fink, Custom installs, etc...) won't they'll already have to update the appropriate library search and library load order options in order to make dependent libraries and binaries built from them work? Does MacPorts do some magic to obscure this requirement from its users? After all is said and done, if the custom library search order is a feature you definitely want back, I will happily add it back at your request. Just say you want the feature and it will be done. I will need your help testing OSX with MacPorts and will likely have one or two follow-up questions along the way. :) FWIW, I'm out of town so I won't be able to hack on this until March 6th or 7th at the earliest, so we have plenty of time to discuss and address any questions or concerns you might have prior to my next available hack session. No rush! :) |
FORWARD: Sometimes text in comments is hard. If you ever feel like chatting on Gitter/IRC is easier... lets do that. To that end, I'm going to have Gitter open more to facilitate that.
I would very much like CI builds under TravisCI/CircleCI to not use willy nilly what are on those boxes already. I want our build for those POSIX systems to install OpenSSL & FreeTDS. To be super clear:
Should have a rake task that works for ports for everyone? This is why I put the scripts to install OpenSSL & FreeTDS in the "test" directory. This is all about CI on those two Ubuntu platforms and making sure we have automated tests using explicit versions. Can this PR change to have explicit versions installed on CI and what is the best way to make that happen? I felt that those simple install scripts were doing that and having a few simple path conventions in ext.rb helped with that on Ubuntu. |
To sum up the results of a revealing gitter conversation between @metaskills and I. I'm going to do the following:
Using all the above verify that |
@metaskills when you get some time would you mind testing this branch on OSX? I think it should work for you now with your custom libraries in /opt/local. Please let me know if I'm wrong. In the mean time I'll be working on getting the windows build working again. :) |
Sure, gimme a few. |
Should this branch include any changes to the readme? |
@aharpervc oh definitely. This is still a work in progress. I'll be updating the README.md as a last step. This is in no way ready-to-merge yet. |
Finally got the Rails v5.1 adapter work done!!! Woohoo. Thanks so much for adding those script back in. I still have to do this to get the default rake task to build/test. Full Log
$ bundle exec rake -- \
--with-freetds-include=/opt/local/include/freetds \
--with-freetds-lib=/opt/local/lib And here is what I saw on the ports build run bundle exec rake ports build``` bundle exec rake ports build tasks/ports.rake:9: warning: already initialized constant OpenSSL::SSL::VERIFY_PEER Compiling ports for x86_64-apple-darwin16.4.0... Downloading openssl-1.1.0e.tar.gz (100%) Extracting openssl-1.1.0e.tar.gz into tmp/x86_64-apple-darwin16.4.0/ports/openssl/1.1.0e... OK Running 'configure' for openssl 1.1.0e... OK Running 'compile' for openssl 1.1.0e... ERROR, review '/Users/kencollins/Repositories/tiny_tds/tmp/x86_64-apple-darwin16.4.0/ports/openssl/1.1.0e/compile.log' to see what happened. Last lines are: ======================================================================== crypto/aes/aes-x86_64.s:1293:1: error: unknown directive .type AES_cbc_encrypt,@function ^ crypto/aes/aes-x86_64.s:1297:1: error: unknown directive .hidden asm_AES_cbc_encrypt ^ crypto/aes/aes-x86_64.s:1748:1: error: unknown directive .size AES_cbc_encrypt,.-AES_cbc_encrypt ^ crypto/aes/aes-x86_64.s:1749:8: error: invalid alignment value .align 64 ^ crypto/aes/aes-x86_64.s:2139:8: error: invalid alignment value .align 64 ^ crypto/aes/aes-x86_64.s:2534:8: error: invalid alignment value .align 64 ^ make[1]: *** [crypto/aes/aes-x86_64.o] Error 1 make: *** [all] Error 2 ======================================================================== rake aborted! Failed to complete compile task /Users/kencollins/Repositories/tiny_tds/tasks/ports/openssl.rb:40:in `execute' /Users/kencollins/Repositories/tiny_tds/tasks/ports/recipe.rb:12:in `cook' tasks/ports.rake:23:in `block (2 levels) in ' tasks/ports.rake:63:in `block (3 levels) in ' tasks/ports.rake:62:in `each' tasks/ports.rake:62:in `block (2 levels) in ' /Users/kencollins/.rbenv/versions/2.4.0/bin/bundle:22:in `load' /Users/kencollins/.rbenv/versions/2.4.0/bin/bundle:22:in `' Tasks: TOP => ports:openssl ```I will find some time to see if I can recommend a diff to help sort this out tomorrow. |
FYI, this works for me and is how the old code was setup. Notice how it sheds an extra # Add all the special path searching from the original tiny_tds build
# order is important here! First in, last searched.
dir_config(
'freetds',
['/opt/local/include', '/opt/local/include/freetds', '/usr/local/include', '/usr/local/include/freetds'],
['/opt/local/lib', '/usr/local/lib']
) Any though on the error I posted above for |
I'll take a look later this week. The successful build is actually misleading. It works in CI but still has errors for local builds. I'm trying to find a compromise that works on Windows and on posix like systems. Thanks for the suggestions, I'll review as soon as I can. I'd like to try and catch you on gitter soon to talk through some tradeoffs I'm working through which are complicating things. Do me a favor and message me on gitter if you happen to be on any day after Saturday? |
I had a few seconds to check and though I'd write it slightly differently, I don't see why we can't check for both include and libdirs which use a freetds prefix. May as well give it a go if it reduces potential for developer error. Turns out there are so many potential permutations that your environment is just different than mine, which is why I value your testing so much! Thanks again for following up. I'd still like to chat on Gitter if possible about windows builds and the related loading errors (which currently this branch still expresses) |
Most any time man! I'm on/off this weekend... around more during the week too. Just start a convo and well see where we land. Thanks agian! |
Both native Darwin and gem:windows worked! Only one issue. I had to remove I published v2.0.0.pre1 to rubygems https://rubygems.org/gems/tiny_tds/versions I suggest I change the adapter to use these and see what happens. If that is clean, I suggest we merge. |
That's curious that byebug caused a windows build failure. Removing though at your request. |
The removal of auto builds for freetds, libiconv, and openssl adds a hard dependency on all deployments and has the potential to break builds. As a result bump a major version to signal a lack of backwards compatibility with previously working gem builds.
Removed byebug and squashed in preparation for merge. The docs probably still need review. |
✅ Adapter PR merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs probably still need review.
Made a small comment on one section. What do you think about adding a few lines about making sure RubyInstaller is down for Windows in the Install section?
README.md
Outdated
$ bundle exec rake ports build | ||
$ bundle exec rake ports test | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this section.
The extension build currently downloads and installs sources for libiconv, openssl, and freetds regardless of architecture. This is at odds with the correct procedure of installing these sources via the OS package manager for the non-binary gem build. This commit begins to solve this problem in a few ways: 1. It moves the responsibiliy of managing the MiniPortile builds to a ports rake task 2. It removes all ports specific logic from the extension build save the ability to search the ports directories if they exist (to facilitate the binary gem) 3. It adds cross compilation targets for the ports rake tasks to assist in the creation of the binary gems. 4. It enhances the rake clobber list to include the ports directories. 5. It removes custom logic for ports directory names and moves back towards relying on the MiniPortile defaults as much as possible. Relates to issue #342
@metaskills I've updated README.md with your suggestions. Let me know if it looks ok. :) |
@metaskills How did the test with the adapter go? Are we good to merge? :) |
GREAT!
YES!!! Would you take the honors? |
Sure thing. I'll merge now. :) |
Merged and in master. Thanks again for sticking with me across this multi-month PR. :) |
Thank you! I just built and punished v2.0.0 gems. |
As per #342 we don't want to automatically build and install FreeTds on all architectures. This should only be needed for the windows fat binary and possibly for local builds during development.
This PR does the following:
rake ports
task which will build all the MiniPortile sources for use in a local build preventing the sorts of failures we saw in issue#340 and PR#341rake ports:cross
task which will cross-compile the ports for our windows target architecturesThe fourth point is worth some discussion. It definitely simplifies things on this end, and with ruby-installer and rails-installer getting closer to an up-to-date openssl build it doesn't seem that big a deal if the wrong one gets loaded. That said, once we go to OpenSSL 1.1.0 if it causes problems, we may need to revisit this portion of the change. I'm hoping will go smoothly.