Skip to content

Conversation

@frozencemetery
Copy link
Member

This is mostly gunk around how the webserver is called and what is
built-in versus a module. I have mostly added templating logic for
commenting pieces of the conf file.

Signed-off-by: Robbie Harwood rharwood@redhat.com

Right now, master is not passing the test suite on all systems (see #117), so this should perhaps wait to merge.

@tjaalton, this may be relevant to your interests.

tests/httpd.conf Outdated
${NODEBIAN}LoadModule unixd_module modules/mod_unixd.so
LoadModule userdir_module modules/mod_userdir.so
LoadModule version_module modules/mod_version.so
${NODEBIAN}LoadModule version_module modules/mod_version.so
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't really use those NODEBIAN modules, perhaps we could simply comment those out and simplify it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that as well, though I'm not sure to what degree they're actually used. I've commented them because they're built-in rather than separate modules on Debian, so they still could be being used there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I comment the modules out, the test suite stops passing on Fedora.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry then.
Looks fine otherwise.

tests/httpd.conf Outdated
${NODEBIAN}LoadModule unixd_module modules/mod_unixd.so
LoadModule userdir_module modules/mod_userdir.so
LoadModule version_module modules/mod_version.so
${NODEBIAN}LoadModule version_module modules/mod_version.so
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry then.
Looks fine otherwise.

distro = "Debian"
moddir = "/usr/lib/apache2/modules"
if not os.path.exists(moddir):
distro = ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above line seem extra

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relic of earlier design. Fixed, thanks.

'HTTPPORT': WRAP_HTTP_PORT})
'HTTPPORT': WRAP_HTTP_PORT,
'NODEBIAN': "#" if distro == "Debian" else "",
'NOFEDORA': "#" if distro == "Fedora" else ""})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this line isn't needed either (sorry for the broken review).

@iboukris
Copy link
Contributor

iboukris commented Dec 21, 2016

BTW, maybe the LoadModule issue could be also solved with IfModule condition, like:

<IfModule !version_module> LoadModule version_module modules/mod_version.so </IfModule>

This is mostly gunk around how the webserver is called and what is
built-in versus a module.  I have mostly added templating logic for
commenting pieces of the conf file.

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
`make test` continues to be provided for compatibility.

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
@frozencemetery
Copy link
Member Author

frozencemetery commented Dec 22, 2016 via email

Copy link
Contributor

@iboukris iboukris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

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.

2 participants