couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jan Lehnardt <...@apache.org>
Subject Re: git commit: hook mochiweb `make` into CouchDB make
Date Mon, 19 Nov 2012 11:42:25 GMT
Thanks for the review, Paul. 


As a preface I need to apologise for not making my intentions with
this branch more clear. My objective here is to figure out how to
bundle Mochiweb in a way that requires minimal adjustments to the
imported source.

e.g. we are currently writing our own makefile for Mochiweb when 
it ships with one that is perfectly fine™.

When looking into things I found that Mochiweb uses rebar and I though
I can use this branch to figure out how use sub-build systems that
hook into our larger Autotools setup. With the help of Noah I got
something to *build*.

More notes inline.


On Nov 19, 2012, at 02:44 , Paul Davis <paul.joseph.davis@gmail.com> wrote:

> On Sun, Nov 18, 2012 at 3:40 PM, <jan@apache.org> wrote:
> 
>> Updated Branches:
>>  refs/heads/1598-update-mochiweb-2-3-2 6fdb9e076 -> 4067b9079
>> 
>> 
>> hook mochiweb `make` into CouchDB make
>> 
>> 
>> Project: http://git-wip-us.apache.org/repos/asf/couchdb/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/couchdb/commit/4067b907
>> Tree: http://git-wip-us.apache.org/repos/asf/couchdb/tree/4067b907
>> Diff: http://git-wip-us.apache.org/repos/asf/couchdb/diff/4067b907
>> 
>> Branch: refs/heads/1598-update-mochiweb-2-3-2
>> Commit: 4067b90798cc52c3a97385607b0d46ab6fb9bafa
>> Parents: 6fdb9e0
>> Author: Jan Lehnardt <jan@apache.org>
>> Authored: Sun Nov 18 21:40:38 2012 +0100
>> Committer: Jan Lehnardt <jan@apache.org>
>> Committed: Sun Nov 18 21:40:38 2012 +0100
>> 
>> ----------------------------------------------------------------------
>> src/Makefile.am          |   88 ++++++++++++++++++++++++++++++++++++++++-
>> src/mochiweb/Makefile.am |   29 +++++++++++++
>> 2 files changed, 116 insertions(+), 1 deletions(-)
>> ----------------------------------------------------------------------
>> 
>> 
>> 
>> http://git-wip-us.apache.org/repos/asf/couchdb/blob/4067b907/src/Makefile.am
>> ----------------------------------------------------------------------
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index fbd514c..4ec74de 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -19,5 +19,91 @@ SUBDIRS = \
>>     erlang-oauth \
>>     etap \
>>     ibrowse \
>> -    mochiweb \
>>     snappy
>> +
>> +mochiweb_file_collection = \
>> +    mochiweb/src/mochifmt.erl \
>> +    mochiweb/src/mochifmt_records.erl \
>> +    mochiweb/src/mochifmt_std.erl \
>> +    mochiweb/src/mochiglobal.erl \
>> +    mochiweb/src/mochihex.erl \
>> +    mochiweb/src/mochijson.erl \
>> +    mochiweb/src/mochijson2.erl \
>> +    mochiweb/src/mochilists.erl \
>> +    mochiweb/src/mochilogfile2.erl \
>> +    mochiweb/src/mochinum.erl \
>> +    mochiweb/src/mochitemp.erl \
>> +    mochiweb/src/mochiutf8.erl \
>> +    mochiweb/src/mochiweb.app.src \
>> +    mochiweb/src/mochiweb.erl \
>> +    mochiweb/src/mochiweb_acceptor.erl \
>> +    mochiweb/src/mochiweb_charref.erl \
>> +    mochiweb/src/mochiweb_cookies.erl \
>> +    mochiweb/src/mochiweb_cover.erl \
>> +    mochiweb/src/mochiweb_echo.erl \
>> +    mochiweb/src/mochiweb_headers.erl \
>> +    mochiweb/src/mochiweb_html.erl \
>> +    mochiweb/src/mochiweb_http.erl \
>> +    mochiweb/src/mochiweb_io.erl \
>> +    mochiweb/src/mochiweb_mime.erl \
>> +    mochiweb/src/mochiweb_multipart.erl \
>> +    mochiweb/src/mochiweb_request.erl \
>> +    mochiweb/src/mochiweb_request_tests.erl \
>> +    mochiweb/src/mochiweb_response.erl \
>> +    mochiweb/src/mochiweb_socket.erl \
>> +    mochiweb/src/mochiweb_socket_server.erl \
>> +    mochiweb/src/mochiweb_util.erl \
>> +    mochiweb/src/reloader.erl
>> +
>> +mochiwebebin_make_generated_file_list = \
>> +    mochiweb/ebin/mochifmt.beam \
>> +    mochiweb/ebin/mochifmt_records.beam \
>> +    mochiweb/ebin/mochifmt_std.beam \
>> +    mochiweb/ebin/mochiglobal.beam \
>> +    mochiweb/ebin/mochihex.beam \
>> +    mochiweb/ebin/mochijson.beam \
>> +    mochiweb/ebin/mochijson2.beam \
>> +    mochiweb/ebin/mochilists.beam \
>> +    mochiweb/ebin/mochilogfile2.beam \
>> +    mochiweb/ebin/mochinum.beam \
>> +    mochiweb/ebin/mochitemp.beam \
>> +    mochiweb/ebin/mochiutf8.beam \
>> +    mochiweb/ebin/mochiweb.app \
>> +    mochiweb/ebin/mochiweb.beam \
>> +    mochiweb/ebin/mochiweb_acceptor.beam \
>> +    mochiweb/ebin/mochiweb_charref.beam \
>> +    mochiweb/ebin/mochiweb_cookies.beam \
>> +    mochiweb/ebin/mochiweb_cover.beam \
>> +    mochiweb/ebin/mochiweb_echo.beam \
>> +    mochiweb/ebin/mochiweb_headers.beam \
>> +    mochiweb/ebin/mochiweb_html.beam \
>> +    mochiweb/ebin/mochiweb_http.beam \
>> +    mochiweb/ebin/mochiweb_io.beam \
>> +    mochiweb/ebin/mochiweb_mime.beam \
>> +    mochiweb/ebin/mochiweb_multipart.beam \
>> +    mochiweb/ebin/mochiweb_request.beam \
>> +    mochiweb/ebin/mochiweb_response.beam \
>> +    mochiweb/ebin/mochiweb_socket.beam \
>> +    mochiweb/ebin/mochiweb_socket_server.beam \
>> +    mochiweb/ebin/mochiweb_util.beam \
>> +    mochiweb/ebin/reloader.beam
>> +
>> +mochiwebebindir = $(localerlanglibdir)/mochiweb-2.3.2/ebin
>> +mochiwebebin_DATA = \
>> +    $(mochiwebebin_make_generated_file_list)
>> +
>> +
>> +$(mochiwebebin_make_generated_file_list): mochiweb.stamp
>> +       @if test -f $@; then :; else \
>> +               rm -f mochiweb.stamp; \
>> +               $(MAKE) $(AM_MAKEFLAGS) mochiweb.stamp; \
>> +       fi
>> +
>> +mochiweb.stamp: $(mochiweb_file_collection)
>> +       @rm -f mochiweb.tmp
>> +       @touch mochiweb.tmp
>> +       make -C mochiweb all
>> +       @mv -f mochiweb.tmp $@
>> +
>> +DISTCLEANFILES = \
>> +       mochiweb.stamp
>> 
>> 
> This should go into src/mochiweb/Makefile.am

The idea is to try and modify the Mochiweb sources as little as possible.
Noah agreed that the Make code that manages the src/mochiweb/ subtree
that has it’s own build system in src/Makefile.am.

An alternative would be src/mochiweb/Makefile.am and then have the
Mochiweb source in src/mochiweb/vendor or something.

Another alternative is realising that the no-modification can only go so
far and alter Mochiweb’s Makefile.


>> http://git-wip-us.apache.org/repos/asf/couchdb/blob/4067b907/src/mochiweb/Makefile.am
>> ----------------------------------------------------------------------
>> diff --git a/src/mochiweb/Makefile.am b/src/mochiweb/Makefile.am
>> new file mode 100644
>> index 0000000..9de1944
>> --- /dev/null
>> +++ b/src/mochiweb/Makefile.am
>> @@ -0,0 +1,29 @@
>> +
>> +PREFIX:=../
>> +DEST:=$(PREFIX)$(PROJECT)
>> +
>> +REBAR=./rebar
>> +
>> +all:
>> +       @$(REBAR) get-deps compile
>> 
> 
> We should absolutely not be calling get-deps and friends. Its a very strict
> rule that code used to build Apache release be stored in Apache version
> control and cleared through the IP hoops that are necessary. Granted I did
> look and mochiweb's rebar.config doesn't declare deps, but we should
> probably get into the habit of disallowing this if we end up bumping into
> something that does try and pull something.

Again, this is just plain Mochiweb’s Makefile and the goal of this is to
figure out how to get it integrated. It is good to know though that we need
to be careful with get-deps in the future.


>> +
>> +edoc:
>> +       @$(REBAR) doc
>> +
>> +test:
>> +       @rm -rf .eunit
>> +       @mkdir -p .eunit
>> +       @$(REBAR) skip_deps=true eunit
>> +
>> +clean:
>> +       @$(REBAR) clean
>> +
>> +build_plt:
>> +       @$(REBAR) build-plt
>> +
>> +dialyzer:
>> +       @$(REBAR) dialyze
>> +
>> +app:
>> +       @$(REBAR) create template=mochiwebapp dest=$(DEST) appid=$(PROJECT)
>> +
>> 
> 
> Other things that you'll want to check into with the updated dependency is
> if all of our changes to mochijson2 have been incorporated (I think most
> have, but I haven't followed up to be certain). We don't want to regress on
> those hard fought bug fixes to UTF-8 validation. :)

Yup, that’s on my todo list.


> I'm also pretty sure that this won't pass make distcheck. We'll either need
> to figure something out to make rebar understand VPATH builds or replace it
> with something of our own design.

Yes, this is very much a work in progress, so far the only thing that works
is that the .beam files are built correctly :)


> Also, checking in rebar we may want to
> ask legal@ what the ruels are on including binary deps inline. I don't
> think it'd be an issue but I can't say that I've ever seen the issue raised
> to know what they'd say.

Benoit brought that up on IRC. I don’t know enough about rebar and whether
a particular build/config is tied to a version, but we might have to look
into per-dependency-version-from-source builds of rebar if this ever lands
in a release.


> We might also want to get rid of the templates and examples included by
> mochiweb. I don't expect we'll ever use them and their license status is
> ambiguous. Rather than bother trying to figure out the hoops I'd just say
> that we should delete them and call it a day.

Again, minimal source mods if possible. My ideal for updating Mochiweb is
this:

    $ cd src/mochiweb
    $ git pull # or some other git magic that fetches the latest release
    $ cd ..
    $ git commit -am 'Update Mochiweb to $VERSION'

If we end up deleting stuff, we may want to write a script to prune and
modify the Mochiweb sources so we don’t have to do it manually.


> +1 on the general source tree refactor.

Again, thanks for the review and sorry for not being too clear with my
intentions! :)

Cheers
Jan
-- 





Mime
View raw message