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

Jump to bottom

libpromhttp: init at 0.1.1 #91323

Closed
wants to merge 1 commit into from
Closed

libpromhttp: init at 0.1.1 #91323

wants to merge 1 commit into from

Conversation

cfsmp3
Copy link

@cfsmp3 cfsmp3 commented Jun 23, 2020

Motivation for this change

Build the HTTP endpoint for prometheus C library. It's needed to expose the metrics.

Things done

Simple derivation that is based on its sister "libprom", pretty much identical to it except for the dependency to libprom itself.

It's an addition, not a change, so it does not break anything.

  • Built on platform(s)

    • NixOS
    • macOS
    • other Linux distributions

  • Determined the impact on package closure size (by running nix path-info -S before and after)

  • Ensured that relevant documentation is up to date

  • Fits CONTRIBUTING.md.

@cfsmp3 cfsmp3 requested review from jtojnar and cole-h June 23, 2020 01:09
cole-h
cole-h approved these changes Jun 23, 2020
View reviewed changes
Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

Diff LGTM, builds fine.

[5 built, 2 copied (0.3 MiB), 0.1 MiB DL]
https://github.com/NixOS/nixpkgs/pull/91323
1 package built:
libpromhttp

@jtojnar
Copy link
Contributor

jtojnar commented Jun 23, 2020

Hmm, looks like it will use the headers from the prom directory:

https://github.com/digitalocean/prometheus-client-c/blob/0c15e7e45ad0c3726593591fdd7d8f2fde845fe3/promhttp/CMakeLists.txt#L49

Maybe we should delete it so it has to choose ones from the package – of course the proper solution is linking against a target obtained by find_package but not sure if you are up for that.

@cfsmp3
Copy link
Author

cfsmp3 commented Jun 23, 2020

Maybe we should delete it so it has to choose ones from the package – of course the proper solution is linking against a target obtained by find_package but not sure if you are up for that.

If you mean by sending digitalocean a PR to improve their CMakelist, probably not at least immediately. It's just too big a detour from my current to-do.

@cfsmp3
Copy link
Author

cfsmp3 commented Jun 23, 2020

@jtojnar could this be merged?

@purcell purcell mentioned this pull request Jul 1, 2020
Merged
10 tasks
@purcell
Copy link
Member

purcell commented Jul 1, 2020

Collaborating with @cfsmp3, this has been reformulated as #91908, which should then probably be preferred to this PR.

@cole-h
Copy link
Member

cole-h commented Jul 2, 2020

In lieu of the above comment, I'll close this PR and review the other one :-)

@cole-h cole-h closed this Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cole-h cole-h cole-h approved these changes

@jtojnar jtojnar Awaiting requested review from jtojnar

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cfsmp3 @jtojnar @purcell @cole-h