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

pdns: remove duplicated dns2_tolower() function #5212

Merged
merged 5 commits into from Apr 11, 2017

Conversation

Projects
None yet
4 participants
@tfarina
Contributor

tfarina commented Mar 29, 2017

There is already a version of dns2_tolower() in misc.hh, called
simply dns_tolower(), but due to some inclusion issues it was necessary
to extract it into another header file (named ascii.hh).

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Mar 31, 2017

Member

Travis is unhappy with your patch. Can you check the logs and find out why?

Member

Habbie commented Mar 31, 2017

Travis is unhappy with your patch. Can you check the logs and find out why?

@tfarina

This comment has been minimized.

Show comment
Hide comment
@tfarina

tfarina Mar 31, 2017

Contributor

I know why it is unhappy, but I don't know autoconf well enough to change the Makefiles to add ascii.hh to the include path so the compiler could find it.

Contributor

tfarina commented Mar 31, 2017

I know why it is unhappy, but I don't know autoconf well enough to change the Makefiles to add ascii.hh to the include path so the compiler could find it.

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Mar 31, 2017

Member

You just need symlinks in the recursordist and dnsdistdist dirs.

Member

Habbie commented Mar 31, 2017

You just need symlinks in the recursordist and dnsdistdist dirs.

@tfarina

This comment has been minimized.

Show comment
Hide comment
@tfarina

tfarina Mar 31, 2017

Contributor

Done. Symlinks added.

Contributor

tfarina commented Mar 31, 2017

Done. Symlinks added.

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Apr 1, 2017

Member

Travis still unhappy (I did not check why).

Member

Habbie commented Apr 1, 2017

Travis still unhappy (I did not check why).

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Apr 1, 2017

Member

Also, if you do requested fixes, please push additional commits, this makes review easier.

Member

Habbie commented Apr 1, 2017

Also, if you do requested fixes, please push additional commits, this makes review easier.

@tfarina

This comment has been minimized.

Show comment
Hide comment
@tfarina

tfarina Apr 1, 2017

Contributor

dnsname.hh:37:20: fatal error: ascii.hh: No such file or directory

Contributor

tfarina commented Apr 1, 2017

dnsname.hh:37:20: fatal error: ascii.hh: No such file or directory

@rgacogne

This comment has been minimized.

Show comment
Hide comment
@rgacogne

rgacogne Apr 2, 2017

Member

So, you need to add ascii.hh to the dnsdist_SOURCES variable in pdns/dnsdistdist/Makefile.am and the pdns_recursor_SOURCES one in pdns/recursordist/Makefile.am, otherwise they are not included in the dnsdist and recursor tarballs.

Member

rgacogne commented Apr 2, 2017

So, you need to add ascii.hh to the dnsdist_SOURCES variable in pdns/dnsdistdist/Makefile.am and the pdns_recursor_SOURCES one in pdns/recursordist/Makefile.am, otherwise they are not included in the dnsdist and recursor tarballs.

@tfarina

This comment has been minimized.

Show comment
Hide comment
@tfarina

tfarina Apr 2, 2017

Contributor

Done. It passed the trybots. Thanks Remi.

Contributor

tfarina commented Apr 2, 2017

Done. It passed the trybots. Thanks Remi.

@pieterlexis

Small nit

Show outdated Hide outdated pdns/Makefile.am

tfarina added some commits Mar 29, 2017

pdns: remove duplicated dns2_tolower() function
There is already a version of dns2_tolower() in misc.hh, called
simply dns_tolower(), but due to some inclusion issues it was necessary
to extract it into another header file (named ascii.hh).
@tfarina

This comment has been minimized.

Show comment
Hide comment
@tfarina

tfarina Apr 7, 2017

Contributor

Done.

Contributor

tfarina commented Apr 7, 2017

Done.

@pieterlexis

This comment has been minimized.

Show comment
Hide comment
@pieterlexis

pieterlexis Apr 7, 2017

Member

hi @tfarina,

Pleas ealso do this to the other Makefile.am's from this PR. They have the same issue 😉

Member

pieterlexis commented Apr 7, 2017

hi @tfarina,

Pleas ealso do this to the other Makefile.am's from this PR. They have the same issue 😉

@tfarina

This comment has been minimized.

Show comment
Hide comment
@tfarina

tfarina Apr 7, 2017

Contributor

Done.

Contributor

tfarina commented Apr 7, 2017

Done.

@pieterlexis pieterlexis merged commit a02b592 into PowerDNS:master Apr 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment