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

Gate ostree-trivial-httpd on BUILDOPT_TRIVIAL_HTTPD #1912

Merged
merged 2 commits into from Oct 16, 2019

Conversation

akiernan
Copy link
Contributor

@akiernan akiernan commented Sep 3, 2019

When building without --enable-trivial-httpd-cmdline, don't build or install
the ostree-trivial-httpd binary.

@rh-atomic-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

@akiernan
Copy link
Contributor Author

akiernan commented Sep 3, 2019

I did wonder when I did this if it was the way it was because of the tests... I guess this means it is.

@cgwalters
Copy link
Member

bot, add author to whitelist

@akiernan akiernan force-pushed the us-fix-trivial-httpd branch 3 times, most recently from 8b4e2bf to 0eea2a1 Compare September 5, 2019 07:13
Makefile-ostree.am Outdated Show resolved Hide resolved
@akiernan
Copy link
Contributor Author

akiernan commented Sep 5, 2019

bot, retest this please

@akiernan
Copy link
Contributor Author

akiernan commented Sep 9, 2019

I'm a bit confused on this... the f29 failures look totally unrelated to me - is this good to merge, or is there something I'm missing?

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

If it's that easy, sure; but did you test make check etc?

From a quick look, I think we'd need all of the tests which use OSTREE_HTTPD to end up skipping if it's unset or so. This would be a lot...better to refactor the tests so that we also use a basic webserver like Python's or whatever, and only require trivial-httpd for the tests that use its tweaks like --random-500s=50.

configure.ac Outdated Show resolved Hide resolved
Makefile-ostree.am Outdated Show resolved Hide resolved
@akiernan
Copy link
Contributor Author

akiernan commented Sep 9, 2019

If it's that easy, sure; but did you test make check etc?

That was where I went wrong initially... so I added a initial commit which pushes --enable-trivial-httpd-cmdline into all the test paths; maybe that's cheating...

Right now the combinations which get covered by Travis for make check pass.

better to refactor the tests so that we also use a basic webserver like Python's or whatever, and only require trivial-httpd for the tests that use its tweaks like --random-500s=50.

That's quite a lot more than I was really planning to do. I wonder if anyone's really using trivial-httpd, or if it's just being used for the tests?

@cgwalters
Copy link
Member

That was where I went wrong initially... so I added a initial commit which pushes --enable-trivial-httpd-cmdline into all the test paths; maybe that's cheating...
Right now the combinations which get covered by Travis for make check pass.

Hmm, OK. Will have to look to see how that's working.

That's quite a lot more than I was really planning to do. I wonder if anyone's really using trivial-httpd, or if it's just being used for the tests?

Yeah I definitely don't expect you to dive into that. I hope no one is using it besides the tests.

I think it might not actually be too hard though to split out the tests which use the trivial-httpd features.

@akiernan
Copy link
Contributor Author

the f29 failures look totally unrelated to me

This was just the f29 failing in setup, it's working now and I can see real failures...

@akiernan
Copy link
Contributor Author

@rh-atomic-bot r+ 2629328

@rh-atomic-bot
Copy link

@akiernan: 🔑 Insufficient privileges: Collaborator required

When running tests we always need ostree-trivial-httpd, so enable it
unconditionally

Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
When building without --enable-trivial-httpd-cmdline, don't build or install
the ostree-trivial-httpd binary.

Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
@cgwalters
Copy link
Member

/approve
/test sanity

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akiernan, cgwalters

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit c943bf4 into ostreedev:master Oct 16, 2019
@dbnicholson
Copy link
Member

Sorry, but this is broken. BUILDOPT_TRIVIAL_HTTPD says whether you want ostree trivial-httpd to be supported, not whether you want the separate ostree-trivial-httpd libexec program. So, this commit says that both ostree trivial-httpd should be supported and ostree-trivial-httpd should be installed. That's what was happening before, but now the test suite is completely if you don't build with --enable-trivial-httpd-cmdline.

What are you trying to achieve exactly? Why is building ostree-trivial-httpd a problem?

@akiernan
Copy link
Contributor Author

What are you trying to achieve exactly?

Avoiding the libexec helper being installed in all cases. When I started looking at it, I guess I'd not realised that it really was just there as part of the test suite, not something most people will actually want.

Maybe what I really want is just delete it after make install

@dbnicholson
Copy link
Member

Debian puts it in a separate ostree-tests package since it's used by the installed tests.

If you want to not install it at all (which would break the installed tests, but probably you don't care), then you could add ostree-trivial-httpd to check_PROGRAMS so it's only built for the in-tree tests. That still wouldn't match the semantics of the current configure option, though. You'd probably want to add a second one to not install the libexec program.

@akiernan
Copy link
Contributor Author

I guess I waded into this assuming it was a bug... seems like that's not true and it's expected behaviour.

Reverting it seems like the best approach and taking the debian route of packaging it separately.

@akiernan
Copy link
Contributor Author

#1950

openshift-merge-robot added a commit that referenced this pull request Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants