Skip to content

Add documentation about linking in Docker #420

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

Merged
merged 3 commits into from
Nov 6, 2018
Merged

Add documentation about linking in Docker #420

merged 3 commits into from
Nov 6, 2018

Conversation

mlh758
Copy link
Contributor

@mlh758 mlh758 commented Oct 31, 2018

Closes #419

I removed documentation that said TinyTDS would install FreeTDS for you since that seems to have been removed in version 2. I can remove that change if it's unwanted.

@aharpervc
Copy link
Contributor

Questions raised purely from reading this readme and not closely investigating the other issue:

  • if freetds is normally in /usr/local/lib, and tinytds already knows how to load from /usr/local/lib, what is the purpose of setting LD_LIBRARY_PATH to /usr/local/lib?
  • when talking about how to set this up with a multistage docker build, wouldn't it be better to install to another directory than default, so your COPY command is more obviously useful (I get that it's copying from a prior stage to the current stage, but when it's the same folder path in both stages it's less obviously interesting)
  • should the url to extconf.rb be to this repo rather than your fork?

@mlh758
Copy link
Contributor Author

mlh758 commented Nov 1, 2018

  • The extconf.rb file knows where to look, but that only helps you at gem build time. When you move it to another container, that link breaks and the OS will try to fix it, but LD_LIBRARY_PATH needs to be updated to show it where to look.

  • You could, but then your builder container has to be a little more complex and set the Free TDS location environment variable which doesn't really get you anything in this case

  • It definitely should be, yes. I'll fix that now.

@mlh758
Copy link
Contributor Author

mlh758 commented Nov 6, 2018

Are you okay with the changes now?

@coderjoe
Copy link
Contributor

coderjoe commented Nov 6, 2018

Just re-read it. Should we really be suggesting a complete overwrite of LD_LIBRARY_PATH?
We should really just be adding /usr/local/lib to the end of the existing path right?

Something like export LD_LIBRARY_PATH=/usr/local/lib:${LD_LIBRARY_PATH} maybe?

@coderjoe
Copy link
Contributor

coderjoe commented Nov 6, 2018

Alternatively we could suggest installing FreeTDS to /usr/lib instead of /usr/local/lib and then maybe you don't have to change LD_LIBRARY_PATH at all?

@mlh758
Copy link
Contributor Author

mlh758 commented Nov 6, 2018

export LD_LIBRARY_PATH=/usr/local/lib:${LD_LIBRARY_PATH} would be safer, it's just normally blank and I assumed the reader would know to preserve it if necessary. I'll change it just to be safer by default.

I don't know why free tds uses /usr/local/lib instead, but tiny_tds also knows to look there and I'm not sure if changing that on the installed system would cause any other weird side effects.

@coderjoe
Copy link
Contributor

coderjoe commented Nov 6, 2018

Looks good to me. Thanks. 👍

@coderjoe coderjoe merged commit d5e9763 into rails-sqlserver:master Nov 6, 2018
@mlh758
Copy link
Contributor Author

mlh758 commented Nov 6, 2018

Thank you for merging!

@mlh758 mlh758 deleted the issue-17-docker-docs branch November 6, 2018 22:23
sukeerthiadiga pushed a commit to sukeerthiadiga/tiny_tds that referenced this pull request Feb 20, 2019
Make note that LD_LIBRARY_PATH may need to be modified if FreeTDS is built in its default configuration in Docker due to /usr/local/lib not being in the path.

Closes rails-sqlserver#419
aharpervc pushed a commit to aharpervc/tiny_tds that referenced this pull request Apr 9, 2020
Make note that LD_LIBRARY_PATH may need to be modified if FreeTDS is built in its default configuration in Docker due to /usr/local/lib not being in the path.

Closes rails-sqlserver#419
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants