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

allow build in termux #935

Merged
merged 7 commits into from Dec 23, 2016

Conversation

Projects
None yet
4 participants
@ygrek
Contributor

ygrek commented Nov 27, 2016

Collection of small tweaks to allow building in termux
One remaining bit is an "/bin/sh" assumption in Unix, I plan to add TARGET_SHELL variable in config/Makefile and allow override it in ./configure

ygrek added some commits Nov 26, 2016

drop explicit SHELL specification from Makefile
it breaks build on termux where /bin/sh is not available, while make finds the proper shell itself just fine
Show outdated Hide outdated configure
@@ -1066,7 +1066,7 @@ echo "#define OCAML_STDLIB_DIR \"$libdir\"" >> s.h
# Do #! scripts work?
if (SHELL=/bin/sh; export SHELL; (./hashbang || ./hashbang2 || ./hashbang3) >/dev/null); then
if ((./hashbang || ./hashbang2 || ./hashbang3 || printf "#!%s\nexit 1\n" `command -v cat` > hashbang; ./hashbang) >/dev/null); then

This comment has been minimized.

@gasche

gasche Nov 27, 2016

Member

This line seems a bit too long and too complex for its own good. Do I correctly understand that the new part is rewriting the file config/auto-aux/hashbang? Is this an intended side-effect? If you would like to build a new hashbang file programmatically (using command -v instead of assumptions on where cat is), maybe this could be done as a first step, on a new hashbang4 file?

@gasche

gasche Nov 27, 2016

Member

This line seems a bit too long and too complex for its own good. Do I correctly understand that the new part is rewriting the file config/auto-aux/hashbang? Is this an intended side-effect? If you would like to build a new hashbang file programmatically (using command -v instead of assumptions on where cat is), maybe this could be done as a first step, on a new hashbang4 file?

This comment has been minimized.

@ygrek

ygrek Nov 27, 2016

Contributor

I didn't want to introduce a call to chmod, but sure, can generate a fresh file

@ygrek

ygrek Nov 27, 2016

Contributor

I didn't want to introduce a call to chmod, but sure, can generate a fresh file

Show outdated Hide outdated otherlibs/unix/getgr.c
@@ -28,7 +28,7 @@ static value alloc_group_entry(struct group *entry)
Begin_roots3 (name, pass, mem);
name = caml_copy_string(entry->gr_name);
pass = caml_copy_string(entry->gr_passwd);
pass = caml_copy_string(entry->gr_passwd ? entry->gr_passwd : "");

This comment has been minimized.

@gasche

gasche Nov 27, 2016

Member

could you comment on why this is necessary? (Maybe even a comment in the source?)

@gasche

gasche Nov 27, 2016

Member

could you comment on why this is necessary? (Maybe even a comment in the source?)

This comment has been minimized.

@ygrek

ygrek Nov 27, 2016

Contributor

gr_passwd is always NULL, blame android :] Cannot change the signature, so this workaround to not segfault...
Witness :

./gu0_a212@localhost:~/tmp$ gcc gr.c -o gr
./gu0_a212@localhost:~/tmp$ ./gr
gr_name root gr_passwd (null) gr_gid 0 
u0_a212@localhost:~/tmp$ cat gr.c
#include <sys/types.h>
#include <unistd.h>
#include <grp.h>
#include <stdio.h>

int main()
{
  struct group* p = getgrgid(0);
  printf("gr_name %s gr_passwd %s gr_gid %d \n", p->gr_name, p->gr_passwd, p->gr_gid);
  return 0;
}
@ygrek

ygrek Nov 27, 2016

Contributor

gr_passwd is always NULL, blame android :] Cannot change the signature, so this workaround to not segfault...
Witness :

./gu0_a212@localhost:~/tmp$ gcc gr.c -o gr
./gu0_a212@localhost:~/tmp$ ./gr
gr_name root gr_passwd (null) gr_gid 0 
u0_a212@localhost:~/tmp$ cat gr.c
#include <sys/types.h>
#include <unistd.h>
#include <grp.h>
#include <stdio.h>

int main()
{
  struct group* p = getgrgid(0);
  printf("gr_name %s gr_passwd %s gr_gid %d \n", p->gr_name, p->gr_passwd, p->gr_gid);
  return 0;
}
@@ -19,6 +19,9 @@ LIBNAME=unix
EXTRACAMLFLAGS=-nolabels
# dllunix.so particularly requires libm for modf symbols
LDOPTS=$(NATIVECCLIBS)

This comment has been minimized.

@ygrek

ygrek Nov 27, 2016

Contributor

Why this is needed.
dllunix.so references modf which comes from libm, but the way it is built currently it only links to libc.so and libdl.so. On my linux system libc.so has a weak reference to modf and then (I guess) libm.so gets linked due to other reasons and symbol is resolved, but on android libc.so has no reference to modf and even though the binary links in libm.so - dllunix.so doesn't "see" it and cannot find modf symbol

@ygrek

ygrek Nov 27, 2016

Contributor

Why this is needed.
dllunix.so references modf which comes from libm, but the way it is built currently it only links to libc.so and libdl.so. On my linux system libc.so has a weak reference to modf and then (I guess) libm.so gets linked due to other reasons and symbol is resolved, but on android libc.so has no reference to modf and even though the binary links in libm.so - dllunix.so doesn't "see" it and cannot find modf symbol

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 27, 2016

Member

If you don't want to call chmod from here, you could commit an empty hashbang4 file with +x (just have a comment in it to say it will be overwritten by configure) and write to it here.

Member

gasche commented on f8d443e Nov 27, 2016

If you don't want to call chmod from here, you could commit an empty hashbang4 file with +x (just have a comment in it to say it will be overwritten by configure) and write to it here.

@ygrek

This comment has been minimized.

Show comment
Hide comment
@ygrek

ygrek Nov 27, 2016

Contributor

actually I see chmod +x called in tools/Makefile so guess we are safe

Contributor

ygrek commented Nov 27, 2016

actually I see chmod +x called in tools/Makefile so guess we are safe

Show outdated Hide outdated configure
@@ -1066,7 +1066,7 @@ echo "#define OCAML_STDLIB_DIR \"$libdir\"" >> s.h
# Do #! scripts work?
if ((./hashbang || ./hashbang2 || ./hashbang3 || printf "#!%s\nexit 1\n" `command -v cat` > hashbang; ./hashbang) >/dev/null); then
if (printf "#!%s\nexit 1\n" `command -v cat` > hashbang4; chmod +x hashbang4; (./hashbang || ./hashbang2 || ./hashbang3 || ./hashbang4) >/dev/null); then

This comment has been minimized.

@gasche

gasche Nov 27, 2016

Member

Is there a reason for defining hashbang4 on the same line, and not in a separate line before the if test?

@gasche

gasche Nov 27, 2016

Member

Is there a reason for defining hashbang4 on the same line, and not in a separate line before the if test?

This comment has been minimized.

@ygrek

ygrek Nov 27, 2016

Contributor

right, fixed

@ygrek

ygrek Nov 27, 2016

Contributor

right, fixed

@ygrek

This comment has been minimized.

Show comment
Hide comment
@ygrek
Contributor

ygrek commented Nov 27, 2016

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 27, 2016

Contributor
Contributor

shindere commented Nov 27, 2016

@ygrek

This comment has been minimized.

Show comment
Hide comment
@ygrek

ygrek Dec 8, 2016

Contributor

By the way, ocaml build system already uses sh in many places explicitly, so there is no concern if this PR will make build pick up some strange shell and break things (which I feared), but on the contrary - adds more uniformity.

Contributor

ygrek commented Dec 8, 2016

By the way, ocaml build system already uses sh in many places explicitly, so there is no concern if this PR will make build pick up some strange shell and break things (which I feared), but on the contrary - adds more uniformity.

@gasche gasche added the approved label Dec 8, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 8, 2016

Member

With that information, I think this is a good change. That said, I'm not confident enough to merge directly: I would like someone else to approve before merging.

Member

gasche commented Dec 8, 2016

With that information, I think this is a good change. That said, I'm not confident enough to merge directly: I would like someone else to approve before merging.

@@ -28,7 +28,8 @@ static value alloc_group_entry(struct group *entry)
Begin_roots3 (name, pass, mem);
name = caml_copy_string(entry->gr_name);
pass = caml_copy_string(entry->gr_passwd);
/* on some platforms, namely Android, gr_passwd can be NULL - hence this workaround */
pass = caml_copy_string(entry->gr_passwd ? entry->gr_passwd : "");

This comment has been minimized.

@avsm

avsm Dec 13, 2016

Member

Is this worth noting in the ChangeLog explicitly, since it involves password handling? (that a NULL response equates to the empty string)

@avsm

avsm Dec 13, 2016

Member

Is this worth noting in the ChangeLog explicitly, since it involves password handling? (that a NULL response equates to the empty string)

This comment has been minimized.

@ygrek

ygrek Dec 13, 2016

Contributor

FTR I believe this concerns a very small fraction of users given that :

  1. all modern OS do not store password (hash) in /etc/passwd, so programs do not expect gr_passwd to contain anything useful
  2. current behaviour in case of NULL is a crash, so this change makes the program go further (worst case - try login with empty string as a password and fail explicitly)
@ygrek

ygrek Dec 13, 2016

Contributor

FTR I believe this concerns a very small fraction of users given that :

  1. all modern OS do not store password (hash) in /etc/passwd, so programs do not expect gr_passwd to contain anything useful
  2. current behaviour in case of NULL is a crash, so this change makes the program go further (worst case - try login with empty string as a password and fail explicitly)

@gasche gasche merged commit d4cd072 into ocaml:trunk Dec 23, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 23, 2016

Member

I went ahead and merged, and added a Change entry after the fact -- please think of including one next time.

Member

gasche commented Dec 23, 2016

I went ahead and merged, and added a Change entry after the fact -- please think of including one next time.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment