Skip to content

Fix build with ALTSVC enabled and COOKIES disabled - #3717

Closed
sunpoet wants to merge 6 commits into
curl:masterfrom
sunpoet:master
Closed

Fix build with ALTSVC enabled and COOKIES disabled#3717
sunpoet wants to merge 6 commits into
curl:masterfrom
sunpoet:master

Conversation

@sunpoet

@sunpoet sunpoet commented Mar 30, 2019

Copy link
Copy Markdown
Contributor

ALTSVC requires Curl_get_line which is defined in lib/cookie.c inside a #if
check of HTTP and COOKIES. That makes Curl_get_line undefined if COOKIES is
disabled. This is a workaround to define Curl_get_line unconditionally.

ALTSVC requires Curl_get_line which is defined in lib/cookie.c inside a #if
check of HTTP and COOKIES. That makes Curl_get_line undefined if COOKIES is
disabled. This is a workaround to define Curl_get_line unconditionally.
@sunpoet

sunpoet commented Mar 30, 2019

Copy link
Copy Markdown
Contributor Author

This is just the workaround I committed for FreeBSD ports tree. You might want to move it to a new place, e.g. lib/curl_get_line.c and lib/curl_get_line.h.

@danielgustafsson danielgustafsson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm more inclined to move this into a generic utilities file, or a file-specific utilities file. It's not very logical to keep it cookie.c now that it has multiple consumers.

Assuming there is buy-in from other maintainers, I'm happy to pick that up in case you're not interested in pursuing that patch.

@bagder

bagder commented Apr 4, 2019

Copy link
Copy Markdown
Member

I agree with you @danielgustafsson. I was mostly just lazy when I left it there in my alt-svc patch...

@danielgustafsson

Copy link
Copy Markdown
Member

I agree with you @danielgustafsson. I was mostly just lazy when I left it there in my alt-svc patch...

Cool, let's do it. @sunpoet do you want to pursue this patch? If not then I'm fine to pick it up.

@sunpoet

sunpoet commented Apr 6, 2019

Copy link
Copy Markdown
Contributor Author

I agree with you @danielgustafsson. I was mostly just lazy when I left it there in my alt-svc patch...

Cool, let's do it. @sunpoet do you want to pursue this patch? If not then I'm fine to pick it up.

I've prepared the patch which moves Curl_get_line to lib/curl_get_line.[ch].

Comment thread lib/cookie.h Outdated
#include "curl_setup.h"

#include <curl/curl.h>
#include "curl_get_line.h"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This isn't used in the header, is it? I'd rather move it to the implementation file.

Comment thread lib/curl_get_line.c

#include "curl_setup.h"

#include "curl_get_line.h"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should also include the following headers after curl_get_line.h:

#include "curl_memory.h"
/* The last #include file should be: */
#include "memdebug.h"

@bagder bagder added the build label Apr 12, 2019
@bagder

bagder commented Apr 12, 2019

Copy link
Copy Markdown
Member

@sunpoet, are you up to taking this all the way to the finishing line?

@sunpoet

sunpoet commented Apr 12, 2019

Copy link
Copy Markdown
Contributor Author

I just committed the changes suggested by @MarcelRaad and @danielgustafsson. Thanks!

@danielgustafsson

Copy link
Copy Markdown
Member

Pushed squashed and with some minor editorialization to the commit message, thanks for contributing!

@sunpoet

sunpoet commented Apr 20, 2019

Copy link
Copy Markdown
Contributor Author

Thanks!

@lock lock Bot locked as resolved and limited conversation to collaborators Jul 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Development

Successfully merging this pull request may close these issues.

4 participants