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.

Sign up for GitHub

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

citgm-smoker-nobuild seems broken #1028

Open
BethGriggs opened this issue Nov 13, 2023 · 23 comments
Open

citgm-smoker-nobuild seems broken #1028

BethGriggs opened this issue Nov 13, 2023 · 23 comments

Comments

@BethGriggs
Copy link
Member

https://ci.nodejs.org/job/citgm-smoker-nobuild/nodes=rhel8-x64/1548/console fails with:

$ curl -O 'https://nodejs.org/download/release/<!DOCTYPE html><html><head><title>Index of downloadrelease<title><meta name='\''viewport'\'' content='\''width=device-width, initial-scale=1.0'\'' ><meta charset='\''utf-8'\'' ><style type='\''textcss'\''>td { padding-right: 16px; text-align: right; font-family: monospace }td:nth-of-type(1) { text-align: left; overflow-wrap: anywhere }td:nth-of-type(3) { white-space: nowrap } th { text-align: left; } @media(prefers-color-scheme: dark) { body { color: white; background-color:#1c1b22; } a { color: #3391ff; } a:visited { color: #C63B65; } }<style><head><body><h1>Index of downloadrelease<h1><table>

I am guessing it's related to changes to what https://nodejs.org/download/release/ returns - I notice the webpage is now styled.
@richardlau
Copy link
Member

FWIW the job is running:

cd $WORKSPACE && rm -rf *
if [ -n "$DOWNLOAD_LOCATION" ]; then
	DOWNLOAD_DIR="https://nodejs.org/download/$DOWNLOAD_LOCATION/"
else
	DOWNLOAD_DIR="https://nodejs.org/download/release/"
fi
LINK=`curl $DOWNLOAD_DIR | grep $NODE_VERSION | sort -t. -k 1,1n -k 2,2n -k 3,3n | tail -1 | cut -d\" -f 2 | tr -d /` 
case $nodes in
  *-ppcle|*-ppc64le) OS=linux; ARCH=ppc64le; EXT=tar.gz;;
  ubuntu*|debian*|fedora*|centos*|rhel*-x64) OS=linux; ARCH=x64; EXT=tar.gz;;
  osx*) OS=darwin; ARCH=x64; EXT=tar.gz;;
  *-s390x) OS=linux; ARCH=s390x; EXT=tar.gz;;
  aix*) OS=aix; ARCH=ppc64; EXT=tar.gz;;
  win*) OS=win; ARCH=x64; EXT=zip;;
esac
curl -O "$DOWNLOAD_DIR$LINK/node-$LINK-$OS-$ARCH.$EXT"
case $nodes in
  win*) unzip node-$LINK-$OS-$ARCH.$EXT ;;
  *) gzip -cd node-$LINK-$OS-$ARCH.$EXT | tar xf - ;;
esac
mv node-$LINK-$OS-$ARCH node

@targos
Copy link
Member

targos commented Nov 13, 2023

/cc @ovflowd @flakey5

@ovflowd
Copy link
Member

ovflowd commented Nov 13, 2023

Quick stupid question, but are really making a GET request with a huge html snippet in the URL?

@ovflowd
Copy link
Member

ovflowd commented Nov 13, 2023

Or what exactly is the issue here?

@flakey5
Copy link
Member

flakey5 commented Nov 13, 2023

Quick stupid question, but are really making a GET request with a huge html snippet in the URL?

+1 to that and also is this relying on parsing whatever html is returned?

@ovflowd
Copy link
Member

ovflowd commented Nov 13, 2023

(Just trying to understand whay the script is trying to do and what is failing here)

@ovflowd
Copy link
Member

ovflowd commented Nov 13, 2023

Quick stupid question, but are really making a GET request with a huge html snippet in the URL?

+1 to that and also is this relying on parsing whatever html is returned?

The html looks like the index of that path, maybe something is wrong and accidentally it is trying to send the html, I have no idea. If someone can explain what this script is doing and what it is supposed to do

@targos
Copy link
Member

targos commented Nov 13, 2023

The script does this:

curl https://nodejs.org/download/release/ | grep v20 | sort -t. -k 1,1n -k 2,2n -k 3,3n | tail -1 | cut -d\" -f 2 | tr -d /

@targos
Copy link
Member

targos commented Nov 13, 2023 •
edited
Loading

You can compare with direct:

curl https://direct.nodejs.org/download/release/ | grep v20 | sort -t. -k 1,1n -k 2,2n -k 3,3n | tail -1 | cut -d\" -f 2 | tr -d /

@ovflowd
Copy link
Member

ovflowd commented Nov 13, 2023

The script does this:


curl https://nodejs.org/download/release/ | grep v20 | sort -t. -k 1,1n -k 2,2n -k 3,3n | tail -1 | cut -d\" -f 2 | tr -d /

The script might need to be adjusted to accommodate the new html structure of our directory listing pages.

Heh, @flakey5 I did say somewhere we were relying on the format of the directory listing k

@richardlau
Copy link
Member

I suspect the existing script is relying on line breaks in the html listings, but the new page has everything on a single line.

FWIW https://ci.nodejs.org/job/node-test-node-addon-api-new/ does something similar and also failing.

@targos
Copy link
Member

targos commented Nov 13, 2023

It would be unfair (and a lot of time lost in total) to expect everyone who was parsing nginx output to change their scripts if there's a way to fix that in the new infra.

@richardlau
Copy link
Member

I'm confused -- I thought https://nodejs.org/download/release/ was still supposed to be served from the DO/Joyent servers and not R2 so shouldn't have changed?

@ovflowd
Copy link
Member

ovflowd commented Nov 13, 2023

It would be unfair (and a lot of time lost in total) to expect everyone who was parsing nginx output to change their scripts if there's a way to fix that in the new infra.

Oh yeah, definitely agree with you here.

@targos
Copy link
Member

targos commented Nov 13, 2023

I'm confused -- I thought https://nodejs.org/download/release/ was still supposed to be served from the DO/Joyent servers and not R2 so shouldn't have changed?

Only /dist is still served from DO/Joyent. Other routes are served from R2 since Nov 8.

@flakey5
Copy link
Member

flakey5 commented Nov 13, 2023

I suspect the existing script is relying on line breaks in the html listings, but the new page has everything on a single line.

+1 I think this is what it's relying on. Formatted the html response and got it to return this
image

When using the test command @targos pointed out it returns just v20.9.0

@ovflowd
Copy link
Member

ovflowd commented Nov 13, 2023

I suspect the existing script is relying on line breaks in the html listings, but the new page has everything on a single line.

+1 I think this is what it's relying on. Formatted the html response and got it to return this image

When using the test command @targos pointed out it returns just v20.9.0

Do we have any updates here? Or issue to keep track of this work on the worker side of things?

@flakey5
Copy link
Member

flakey5 commented Nov 13, 2023

Working on a fix on the worker side of things here nodejs/release-cloudflare-worker#65

@mhdawson
Copy link
Member

@flakey5 good to hear you are working on it. Let me know when the fix is in place so that I can confirm it fixes the Node-api tests which were also broken (https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/).

@flakey5
Copy link
Member

flakey5 commented Nov 15, 2023

The jobs should be fixed now. We moved the worker off of /download until nodejs/release-cloudflare-worker#74 is resolved due to multiple issues that we've been seeing

@ovflowd
Copy link
Member

ovflowd commented Nov 15, 2023

Well, can we manually test that they will not break once we move them back? We just merged the new directory listing can we check that?

@flakey5
Copy link
Member

flakey5 commented Nov 15, 2023

Once nodejs/release-cloudflare-worker#76 lands this issue should be fixed on the worker's end

@flakey5
Copy link
Member

flakey5 commented Nov 23, 2023

Ran the command against the worker again and it works
image

@flakey5 flakey5 mentioned this issue Nov 28, 2023
Merged
1 task
@flakey5 flakey5 mentioned this issue Jun 26, 2024
Open
@flakey5 flakey5 mentioned this issue Aug 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@targos @richardlau @BethGriggs @mhdawson @ovflowd @flakey5