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

Fixed building on FreeBSD. #754

Closed
wants to merge 3 commits into from
Closed

Fixed building on FreeBSD. #754

wants to merge 3 commits into from

Conversation

ronelliott
Copy link

There is an issue with executing the utils/has_lib.sh script on FreeBSD. This is a fix for it.

@ghost ghost mentioned this pull request Apr 14, 2016
@LinusU
Copy link
Collaborator

LinusU commented Apr 14, 2016

Is bash not available on FreeBSD? Can you change the shebang instead?

@ronelliott
Copy link
Author

@LinusU No, by default bash will not be installed on FreeBSD. Though it is available, it requires installing the package and the shebang will still have to be updated as it's installed to /usr/local/bin rather than /bin. PR has been updated to change the shebang rather than modifying the binding.gyp.

@LinusU
Copy link
Collaborator

LinusU commented Apr 15, 2016

The script will need updating though, on POSIX sh local is undefined...

@ghost
Copy link

ghost commented Apr 15, 2016

What about tcsh?

@zbjornson
Copy link
Collaborator

tcsh is not that common:

Debian GNU/Linux 8.3 (jessie):

$ cat /etc/shells
/bin/sh
/bin/dash
/bin/bash
/bin/rbash
/usr/bin/screen

Ubuntu 15.10:

$ cat /etc/shells
/bin/sh
/bin/dash
/bin/bash
/bin/rbash
/usr/bin/tmux
/usr/bin/screen

Centos 7.2.1511:

$ cat /etc/shells
/bin/sh
/bin/bash
/sbin/nologin
/usr/bin/sh
/usr/bin/bash
/usr/sbin/nologin

Which is to say ... probably /bin/sh, or /bin/bash if not for FreeBSD 😠 😄

@LinusU
Copy link
Collaborator

LinusU commented Apr 15, 2016

OS X 10.11.4

$ cat /etc/shells
# List of acceptable shells for chpass(1).
# Ftpd will not allow users to connect who are not using
# one of these shells.

/bin/bash
/bin/csh
/bin/ksh
/bin/sh
/bin/tcsh
/bin/zsh

Ubuntu 13.10

$ cat /etc/shells
# /etc/shells: valid login shells
/bin/csh
/bin/sh
/usr/bin/es
/usr/bin/ksh
/bin/ksh
/usr/bin/rc
/usr/bin/tcsh
/bin/tcsh
/usr/bin/esh
/bin/dash
/bin/bash
/bin/rbash
/usr/bin/screen
/usr/bin/git-shell

Ubuntu 14.04

$ cat /etc/shells
# /etc/shells: valid login shells
/bin/sh
/bin/dash
/bin/bash
/bin/rbash
/usr/bin/tmux
/usr/bin/screen

@LinusU
Copy link
Collaborator

LinusU commented Apr 19, 2016

Well, we do know that node is available so we could just write the script in JavaScript

@chearon
Copy link
Collaborator

chearon commented Apr 21, 2016

Isn't sh always linked to the right shell? Shouldn't it just be #!/bin/sh?

@LinusU
Copy link
Collaborator

LinusU commented Apr 21, 2016

@chearon The current script won't work in sh though, it's written for bash. That should be easy to fix though...

@zbjornson
Copy link
Collaborator

Check out what's done here using node to detect libjpeg-turbo:
https://github.com/Automattic/node-canvas/pull/458/files
My vote goes to consolidating lib detection into a single node script like that.

@ghost
Copy link

ghost commented Apr 23, 2016

Please do!

@zbjornson
Copy link
Collaborator

Hrm, well...

I ported has_lib.sh to javascript (untested): https://gist.github.com/zbjornson/3959cae50388d83f69317298257195d3

But, I didn't realize that the jpeg lookup script in #458 returns the library path, and has_lib returns true/false, so consolidation is less obvious (but still possible). Without that benefit, switching that library to JS because of scoping seems silly.

As far as modifying has_lib.sh, it looks like result is already being leaked, so I don't see much further harm in just removing the "local" keyword before regex and letting it leak too, perhaps naming it something longer to reduce risk of overwriting an existing var and unset'ing it at the end of the function. To keep the scope local, you could change the { and } to ( and ) and remove the local keyword in has_system_lib, but I'm not sure if the path modification in has_lib.sh needs to be exported for the build to work -- if they do, that change would break it.

@LinusU
Copy link
Collaborator

LinusU commented Apr 25, 2016

Actually, maybe this would be a great time to fix has_lib.sh which isn't really working that good anyways.

Instead of trying a bunch of directories that we think might have the libraries, we should just ask the compiler if it can find it or not. This have been discussed before and I'm still not entirely sure on exactly how to do it, but something like invoking the compiler with e.g. -ljpeg and see if it blows up...

@qbit
Copy link

qbit commented Apr 26, 2016

If you really need to use bash changing the #!/bin/bash line to #!/usr/bin/env bash will solve the problem (assuming bash is installed on the target machine). This issue will also impact OpenBSD machines which typically have bash in /usr/local/bin/bash (but could also be in other locations).

The Single UNIX Specification specifies:

Applications should note that the standard PATH to the shell cannot be assumed to be either /bin/sh or /usr/bin/sh, and should be determined by interrogation of the PATH returned by getconf PATH, ensuring that the returned pathname is an absolute pathname and not a shell built-in.

The specification also states one should define the #! line only at install time, once the proper PATH of a given shell has been determined:

Furthermore, on systems that support executable scripts (the "#!" construct), it is recommended that applications using executable scripts install them using getconf PATH to determine the shell pathname and update the "#!" script appropriately as it is being installed (for example, with sed). For example:

@ghost
Copy link

ghost commented Apr 27, 2016

I agree with @qbit's suggestion to use #!/usr/bin/env bash instead, or skip bash altogether.

Doing ln -s /usr/local/bin/bash /bin/bash fixed the problem temporarily.

Thanks everyone!

@LinusU
Copy link
Collaborator

LinusU commented Apr 27, 2016

#!/usr/bin/env bash sounds fine by me 👍

@piranna
Copy link
Contributor

piranna commented Sep 11, 2016

I like the idea of using Node.js to replace the shell scripts in the long term.

@ghost
Copy link

ghost commented Oct 7, 2016

Might as well drop bash altogether and just use sh which all the major Unix-like OSes have in common, see PR.

Also, for the record:

# cat /etc/shells
#       $OpenBSD: shells,v 1.8 2009/02/14 17:06:40 sobrado Exp $
#
# list of acceptable shells for chpass(1).
# ftpd(8) will not allow users to connect who are not using
# one of these shells, unless the user is listed in /etc/ftpchroot.
/bin/sh
/bin/csh
/bin/ksh

LinusU pushed a commit to LinusU/node-canvas that referenced this pull request Oct 23, 2016
zbjornson added a commit to zbjornson/node-canvas that referenced this pull request Feb 21, 2017
zbjornson added a commit to zbjornson/node-canvas that referenced this pull request Feb 21, 2017
zbjornson added a commit to zbjornson/node-canvas that referenced this pull request Feb 21, 2017
zbjornson added a commit to zbjornson/node-canvas that referenced this pull request Feb 22, 2017
zbjornson added a commit to zbjornson/node-canvas that referenced this pull request Feb 22, 2017
@LinusU
Copy link
Collaborator

LinusU commented May 3, 2017

This script has been replaced with a javascript-script instead, now it should work :)

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.

6 participants