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

Build system: fix a few hardcoded ar commands. #249

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@dbuenzli
Copy link
Contributor

commented Sep 28, 2015

They break cross-compiling scenarios.

A git grep "ar rc" also reveals the following occurences but I don't know exactly if something should be done about it or not:

> git grep "ar rc"
config/Makefile-templ:#ml let mklib out files opts = Printf.sprintf "ar rc %s %s %s; ranlib %s" out opts files out;;
config/Makefile.mingw:MKLIB=rm -f $(1); $(TOOLPREF)ar rc $(1) $(2); $(RANLIB) $(1)
config/Makefile.mingw:#ml let mklib out files opts = Printf.sprintf "rm -f %s && %sar rcs %s %s %s" out toolpref opts out files;;
config/Makefile.mingw64:MKLIB=rm -f $(1); $(TOOLPREF)ar rc $(1) $(2); $(RANLIB) $(1)
config/Makefile.mingw64:#ml let mklib out files opts = Printf.sprintf "rm -f %s && %sar rcs %s %s %s" out toolpref opts out files;;
configure:MKLIB=${TOOLPREF}ar rc \$(1) \$(2); ${TOOLPREF}ranlib \$(1)
configure:#ml   Printf.sprintf "${TOOLPREF}ar rc %s %s %s;

@dbuenzli dbuenzli referenced this pull request Sep 28, 2015

Closed

Failure on darwin #8

@whitequark

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

The references to ar in config/ affect the behavior of ocamlmklib, so yes, they also should be fixed.

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

Ok I'll have a look at that.

@whitequark

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

Oh, and the ones in configure too. These all do the same thing essentially. (The configuration of ocamlmklib is such a horrible hack...)

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

Ugh all this looks very messy would $(VARIABLE)s be substituted on these #ml lines. ?

@whitequark

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

No. But now that I am reading your actual patch again, it seems that all the lines above already run the equivalent of $(ARCMD) and not ar. I mean, that's how it is defined:

configure:979:echo "ARCMD=${TOOLPREF}ar" >> Makefile

So you don't need to do anything with them.

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

Except that one no ?

config/Makefile-templ:#ml let mklib out files opts = Printf.sprintf "ar rc %s %s %s; ranlib %s" out opts files out;;

which should maybe be turned to

config/Makefile-templ:#ml let mklib out files opts = Printf.sprintf "%sar rc %s %s %s; %sranlib %s" toolpref out opts files toolpref out;;

?

@whitequark

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

Oh, yes, good catch. That looks good to me.

@dbuenzli dbuenzli force-pushed the dbuenzli:ar-fix branch from 335d577 to 6fc1cd0 Sep 28, 2015

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 9, 2015

Merged in trunk, thanks for the patch and the review!

@gasche gasche closed this Oct 9, 2015

@dbuenzli dbuenzli deleted the dbuenzli:ar-fix branch Jan 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.