Skip to content

travis: use go stable, (second try) - #4403

Closed
jay wants to merge 3 commits into
masterfrom
bagder/travis-boringssl-go
Closed

jay wants to merge 3 commits into
masterfrom
bagder/travis-boringssl-go

Conversation

@jay

@jay jay commented Sep 23, 2019

Copy link
Copy Markdown
Member

Mac builds have been failing since 52db0b8 was added to use go stable in all builds. Based on the failures it looks like gimme is not available in Mac images? I've written support to find out but also I'd like to see if it's possible to limit getting go latest stable to just the boringssl builds.

Ref: #4361

@jay
jay force-pushed the bagder/travis-boringssl-go branch from 5ad50d9 to 52b264a Compare September 24, 2019 05:19
@jay

jay commented Sep 24, 2019

Copy link
Copy Markdown
Member Author

Is something else affecting the two boringssl builds? i don't see why this

url.c: In function ‘Curl_connect’:
url.c:1883:13: error: offset outside bounds of constant string [-Werror]
     hostname++;

curl/lib/url.c

Lines 1872 to 1888 in 4a778f7

hostname = data->state.up.hostname;
if(!hostname)
/* this is for file:// transfers, get a dummy made */
hostname = (char *)"";
if(hostname[0] == '[') {
/* This looks like an IPv6 address literal. See if there is an address
scope. */
size_t hlen;
conn->bits.ipv6_ip = TRUE;
/* cut off the brackets! */
hostname++;
hlen = strlen(hostname);
hostname[hlen - 1] = 0;
zonefrom_url(uh, conn);
}

confused by (char *)"" maybe. i wonder if it would be confused even if const char *hostname. bad practice anyway so how about char empty[]=""; then later there if (!hostname) hostname = empty;

@bagder

bagder commented Sep 24, 2019

Copy link
Copy Markdown
Member

Try it. And the check for '[' could use an else too...

Attempt to address this:

url.c: In function ‘Curl_connect’:
url.c:1883:13: error: offset outside bounds of constant string [-Werror]
     hostname++;
             ^
@bagder

bagder commented Sep 24, 2019

Copy link
Copy Markdown
Member

That seems to have done it!

@jay

jay commented Sep 24, 2019 •

Copy link
Copy Markdown
Member Author

That seems to have done it!

Yeah but we're still lying to the compiler, bad practice...

In other news support says gimme is not preinstalled on mac VMs so they would need if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew install gimme; fi but I think it's easier to just limit it to the two boringssl builds as above.

how about this

diff --git a/lib/url.c b/lib/url.c
index e4c4793..a6794a3 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -1869,12 +1869,10 @@ static CURLcode parseurlandfillconn(struct Curl_easy *data,
 
   (void)curl_url_get(uh, CURLUPART_QUERY, &data->state.up.query, 0);
 
+  /* note file:// transfers may not have a hostname */
   hostname = data->state.up.hostname;
-  if(!hostname)
-    /* this is for file:// transfers, get a dummy made */
-    hostname = (char *)"";
 
-  if(hostname[0] == '[') {
+  if(hostname && hostname[0] == '[') {
     /* This looks like an IPv6 address literal. See if there is an address
        scope. */
     size_t hlen;
@@ -1888,7 +1886,7 @@ static CURLcode parseurlandfillconn(struct Curl_easy *data,
   }
 
   /* make sure the connect struct gets its own copy of the host name */
-  conn->host.rawalloc = strdup(hostname);
+  conn->host.rawalloc = strdup(hostname ? hostname : "");
   if(!conn->host.rawalloc)
     return CURLE_OUT_OF_MEMORY;
   conn->host.name = conn->host.rawalloc;

comment probably unnecessary now since code speaks for it

@bagder

bagder commented Sep 24, 2019

Copy link
Copy Markdown
Member

sure, that works for me!

@bagder

bagder commented Sep 24, 2019

Copy link
Copy Markdown
Member

I think it's easier to just limit it to the two boringssl builds as above.

For now, yes. I think it would be valuable to add those builds to osx too though so it could be worth testing out that solution. But let's do that in a separate PR.

@bagder bagder closed this in 7c7dac4 Sep 25, 2019
bagder pushed a commit that referenced this pull request Sep 25, 2019
@bagder
bagder deleted the bagder/travis-boringssl-go branch September 25, 2019 12:35
@lock lock Bot locked as resolved and limited conversation to collaborators Dec 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants