varnish / libvmod-example

An example vmod for Varnish Cache
https://varnish-cache.org/vmods/
53 stars 74 forks source link

build requires rst2man even after make dist #25

Closed ingvarha closed 6 years ago

ingvarha commented 9 years ago

make dist when successfully run, produces a configure that unnecessarily requires rst2man, as the manpage is prebuilt by make dist. This makes the release tarball difficult to build on platforms that misses rst2man, like rhel5 and clones.

Workaround: export RST2MAN=/bin/true before calling configure.

See https://github.com/toreanderson/libvmod-rfc6052/issues/2 . vmod-rfc6052 is based on this example vmod, and the bug originates here.

Ingvar

dridi commented 8 years ago

I believe I broke this one, assigning to myself.

dridi commented 8 years ago

Hi Ingvar,

I have started a new VMOD this week and took this as an occasion to clean the build system, including this issue, #26 and #27.

The main changes are:

I believe I also managed to remove all target/source names duplication in the makefiles, without overdoing it. I barely touched configure.ac, only to search the VTC test suite there. I haven't touched the debian directory.

So it works on my machine, but...

If it's OK for the both of you, I'll submit a pull request with similar changes.

Happy New Year, Dridi

[1] $(builddir) no longer appears in the makefiles

ingvarha commented 8 years ago

Hello, Dridi. Nice work!

I have not done any functionality testing, only builds and make check.

Builds fine on fedora. Builds fine on el5, el6, and el7 as well.

The build needs varnish-4.1 to build, but configure does not stop nor warn on varnish-4.0 (default on epel7). It should.

Do you want me to put this up at https://copr.fedoraproject.org/coprs/ingvar/varnish41/ ?

Ingvar

----- On Jan 2, 2016, at 12:43 PM, Dridi Boukelmoune notifications@github.com wrote:

Hi Ingvar,

I have started a new VMOD this week and took this as an occasion to clean the build system, including this issue, #26 and #27 .

The main changes are:

* the use of autotools's test driver 
    * we get the same green PASS and red FAIL as Varnish 
    * I also have unit tests in this module (out of scope) 
* simpler dependencies with generated sources 
    * explicit dependencies between $(srcdir) and $(builddir) [1] 
    * declaration of all the files created by the vmodtool 
    * the man page is generated in the src/ directory 
* a cleanup of the RPM spec file 
* a cleanup of .gitignore 

I believe I also managed to remove all target/source names duplication in the makefiles, without overdoing it. I barely touched configure.ac , only to search the VTC test suite there. I haven't touched the debian directory.

So it works on my machine, but...

* could you please try to build this module on different platforms, including el5? 
* @lkarsten could you also please review it? 

If it's OK for the both of you, I'll submit a pull request with similar changes.

Happy New Year, Dridi

[1] $(builddir) no longer appears in the makefiles

— Reply to this email directly or view it on GitHub .

dridi commented 8 years ago

Hey Ingvar,

I have not done any functionality testing, only builds and make check.

It should work up to make distcheck, and package builds from a make dist tarball. However there is currently no functionality.

The build needs varnish-4.1 to build, but configure does not stop nor warn on varnish-4.0 (default on epel7). It should.

I'm not sure branches of this repo check for the Varnish version in configure, I can fix that too.

As for myself I had only tested the build on a mock cache (fc22) I had on my machine, I was offline and luckily I had done Varnish 4.1 mock builds previously.

The VMOD hasn't been started yet. I'm barely starting with HPACK, so I think it is way too early to consider packaging it in your COPR repo. I'll keep that in mind though :)

Dridi

dridi commented 8 years ago

Going back to the original subject, I'm not sure it's a good idea to include man pages in the distribution. They depend on the vmodtool used at build-time, so I'm not sure it's a good idea to save them at dist-time.

dridi commented 6 years ago

Closing for the reason stated in my previous comment.