Skip to content

Commit dd66541

Browse files
partoufmattgodbolt
partouf
authored and
mattgodbolt
committed
Attempt to fix remote library fetching (compiler-explorer#7303)
Libraries were being fetched from wrong URL (on startup); `Fetching remote libraries from https://godbolt.org:443/api/libraries/c++` Ended up refactoring a bunch of things to get some unittesting going on I did not test actual startup and compilation with CE, I might have broken something. (but @mattgodbolt tested and it looked great)
1 parent a31902d commit dd66541

13 files changed

+97
-16
lines changed

‎lib/compiler-finder.ts‎

Lines changed: 38 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,24 @@ export class CompilerFinder {
8585
return this.awsPoller.getInstances();
8686
}
8787

88+
static prepareRemoteUrlParts(host: string, port: number, uriBase: string, langId: string | null) {
89+
const uriSchema = port === 443 ? 'https' : 'http';
90+
return {
91+
uriSchema: uriSchema,
92+
uri: urljoin(`${uriSchema}://${host}:${port}`, uriBase),
93+
apiPath: urljoin('/', uriBase || '', 'api/compilers', langId || '', '?fields=all'),
94+
};
95+
}
96+
97+
static getRemoteInfo(uriSchema: string, host: string, port: number, uriBase: string, compilerId: string) {
98+
return {
99+
target: `${uriSchema}://${host}:${port}`,
100+
path: urljoin('/', uriBase, 'api/compiler', compilerId, 'compile'),
101+
cmakePath: urljoin('/', uriBase, 'api/compiler', compilerId, 'cmake'),
102+
basePath: urljoin('/', uriBase),
103+
};
104+
}
105+
88106
async fetchRemote(
89107
host: string,
90108
port: number,
@@ -93,9 +111,7 @@ export class CompilerFinder {
93111
langId: string | null,
94112
): Promise<CompilerInfo[] | null> {
95113
const requestLib = port === 443 ? https : http;
96-
const uriSchema = port === 443 ? 'https' : 'http';
97-
const uri = urljoin(`${uriSchema}://${host}:${port}`, uriBase);
98-
const apiPath = urljoin('/', uriBase || '', 'api/compilers', langId || '', '?fields=all');
114+
const {uriSchema, uri, apiPath} = CompilerFinder.prepareRemoteUrlParts(host, port, uriBase, langId);
99115
logger.info(`Fetching compilers from remote source ${uri}`);
100116
return this.retryPromise(
101117
() => {
@@ -150,11 +166,13 @@ export class CompilerFinder {
150166
if (typeof compiler.alias == 'string') compiler.alias = [compiler.alias];
151167
// End fixup
152168
compiler.exe = '/dev/null';
153-
compiler.remote = {
154-
target: `${uriSchema}://${host}:${port}`,
155-
path: urljoin('/', uriBase, 'api/compiler', compiler.id, 'compile'),
156-
cmakePath: urljoin('/', uriBase, 'api/compiler', compiler.id, 'cmake'),
157-
};
169+
compiler.remote = CompilerFinder.getRemoteInfo(
170+
uriSchema,
171+
host,
172+
port,
173+
uriBase,
174+
compiler.id,
175+
);
158176
return compiler;
159177
});
160178
resolve(compilers);
@@ -369,19 +387,25 @@ export class CompilerFinder {
369387
.filter(a => a !== '');
370388
}
371389

390+
static getRemotePartsFromCompilerName(remoteCompilerName: string) {
391+
const bits = remoteCompilerName.split('@');
392+
const pathParts = bits[1].split('/');
393+
return {
394+
host: bits[0],
395+
port: parseInt(unwrap(pathParts.shift())),
396+
uriBase: pathParts.join('/'),
397+
};
398+
}
399+
372400
async recurseGetCompilers(
373401
langId: string,
374402
compilerName: string,
375403
parentProps: CompilerProps['get'],
376404
): Promise<PreliminaryCompilerInfo[]> {
377405
// Don't treat @ in paths as remote addresses if requested
378406
if (this.args.fetchCompilersFromRemote && compilerName.includes('@')) {
379-
const bits = compilerName.split('@');
380-
const host = bits[0];
381-
const pathParts = bits[1].split('/');
382-
const port = parseInt(unwrap(pathParts.shift()));
383-
const path = pathParts.join('/');
384-
return (await this.fetchRemote(host, port, path, this.ceProps, langId)) || [];
407+
const {host, port, uriBase} = CompilerFinder.getRemotePartsFromCompilerName(compilerName);
408+
return (await this.fetchRemote(host, port, uriBase, this.ceProps, langId)) || [];
385409
}
386410
if (compilerName.indexOf('&') === 0) {
387411
const groupName = compilerName.substring(1);

‎lib/options-handler.ts‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import path from 'path';
2828
import fs from 'fs-extra';
2929
import semverParser from 'semver';
3030
import _ from 'underscore';
31+
import urlJoin from 'url-join';
3132

3233
import {AppDefaultArguments} from '../app.js';
3334
import {splitArguments} from '../shared/common-utils.js';
@@ -399,7 +400,7 @@ export class ClientOptionsHandler {
399400
const remoteId = this.getRemoteId(remoteUrl, language);
400401
if (!this.remoteLibs[remoteId]) {
401402
return new Promise(resolve => {
402-
const url = remoteUrl + '/api/libraries/' + language;
403+
const url = ClientOptionsHandler.getRemoteUrlForLibraries(remoteUrl, language);
403404
logger.info(`Fetching remote libraries from ${url}`);
404405
let fullData = '';
405406
https.get(url, res => {
@@ -428,6 +429,14 @@ export class ClientOptionsHandler {
428429
await this.getRemoteLibraries(language, target);
429430
}
430431

432+
static getFullRemoteUrl(remote): string {
433+
return remote.target + remote.basePath;
434+
}
435+
436+
static getRemoteUrlForLibraries(url: string, language: LanguageKey) {
437+
return urlJoin(url, '/api/libraries', language);
438+
}
439+
431440
async setCompilers(compilers: CompilerInfo[]) {
432441
const forbiddenKeys = new Set([
433442
'exe',
@@ -459,7 +468,10 @@ export class ClientOptionsHandler {
459468
}
460469

461470
if (compiler.remote) {
462-
await this.fetchRemoteLibrariesIfNeeded(compiler.lang, compiler.remote.target);
471+
await this.fetchRemoteLibrariesIfNeeded(
472+
compiler.lang,
473+
ClientOptionsHandler.getFullRemoteUrl(compiler.remote),
474+
);
463475
}
464476

465477
for (const propKey of Object.keys(compiler)) {

‎test/analysis-tests.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ describe('LLVM-mca tool definition', () => {
4949
target: 'foo',
5050
path: 'bar',
5151
cmakePath: 'cmake',
52+
basePath: '/',
5253
},
5354
lang: languages.analysis.id,
5455
});
@@ -92,6 +93,7 @@ describe('LLVM-mca tool definition', () => {
9293
target: 'foo',
9394
path: 'bar',
9495
cmakePath: 'cmake',
96+
basePath: '/',
9597
},
9698
lang: 'analysis',
9799
disabledFilters: 'labels,directives,debugCalls' as any,

‎test/base-compiler-tests.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ describe('Basic compiler invariants', () => {
6060
target: 'foo',
6161
path: 'bar',
6262
cmakePath: 'cmake',
63+
basePath: '/',
6364
},
6465
lang: 'c++',
6566
ldPath: [],
@@ -119,6 +120,7 @@ describe('Compiler execution', () => {
119120
target: 'foo',
120121
path: 'bar',
121122
cmakePath: 'cmake',
123+
basePath: '/',
122124
},
123125
lang: 'c++',
124126
ldPath: [],
@@ -132,6 +134,7 @@ describe('Compiler execution', () => {
132134
target: 'foo',
133135
path: 'bar',
134136
cmakePath: 'cmake',
137+
basePath: '/',
135138
},
136139
lang: 'c++',
137140
ldPath: [],
@@ -144,6 +147,7 @@ describe('Compiler execution', () => {
144147
target: 'foo',
145148
path: 'bar',
146149
cmakePath: 'cmake',
150+
basePath: '/',
147151
},
148152
lang: 'c++',
149153
ldPath: [],
@@ -154,6 +158,7 @@ describe('Compiler execution', () => {
154158
target: 'foo',
155159
path: 'bar',
156160
cmakePath: 'cmake',
161+
basePath: '/',
157162
},
158163
lang: 'c++',
159164
ldPath: [],
@@ -748,6 +753,7 @@ describe('getDefaultExecOptions', () => {
748753
target: 'foo',
749754
path: 'bar',
750755
cmakePath: 'cmake',
756+
basePath: '/',
751757
},
752758
lang: 'c++',
753759
ldPath: [],
@@ -818,6 +824,7 @@ describe('Rust overrides', () => {
818824
target: '',
819825
path: '',
820826
cmakePath: '',
827+
basePath: '/',
821828
},
822829
semver: 'nightly',
823830
lang: 'rust',

‎test/d-tests.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ describe('D', () => {
4242
target: 'foo',
4343
path: 'bar',
4444
cmakePath: 'cmake',
45+
basePath: '/',
4546
},
4647
lang: languages.d.id,
4748
};

‎test/golang-tests.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ const info = {
4141
target: 'foo',
4242
path: 'bar',
4343
cmakePath: 'cmake',
44+
basePath: '/',
4445
},
4546
lang: languages.go.id,
4647
};

‎test/library-tests.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ describe('Library directories (c++)', () => {
5050
target: 'foo',
5151
path: 'bar',
5252
cmakePath: 'cmake',
53+
basePath: '/',
5354
},
5455
lang: 'c++',
5556
ldPath: [],
@@ -223,6 +224,7 @@ describe('Library directories (fortran)', () => {
223224
target: 'foo',
224225
path: 'bar',
225226
cmakePath: 'cmake',
227+
basePath: '/',
226228
},
227229
lang: 'fortran',
228230
ldPath: [],

‎test/nim-tests.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ describe('Nim', () => {
4444
target: 'foo',
4545
path: 'bar',
4646
cmakePath: 'cmake',
47+
basePath: '/',
4748
},
4849
lang: languages.nim.id,
4950
};

‎test/odin-tests.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ const info = {
4141
target: 'example',
4242
path: 'dummy',
4343
cmakePath: 'cmake',
44+
basePath: '/',
4445
},
4546
lang: languages.odin.id,
4647
};

‎test/options-handler.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import {beforeAll, describe, expect, it} from 'vitest';
3030
import {AppDefaultArguments} from '../app.js';
3131
import {BaseCompiler} from '../lib/base-compiler.js';
3232
import {CompilationEnvironment} from '../lib/compilation-env.js';
33+
import {CompilerFinder} from '../lib/compiler-finder.js';
3334
import {ClientOptionsHandler, ClientOptionsType} from '../lib/options-handler.js';
3435
import * as properties from '../lib/properties.js';
3536
import {BaseTool} from '../lib/tooling/base-tool.js';
@@ -566,4 +567,30 @@ describe('Options handler', () => {
566567
},
567568
});
568569
});
570+
571+
it('should correctly resolve remote urls', () => {
572+
const compilerName = 'godbolt.org@443/gpu';
573+
const {host, port, uriBase} = CompilerFinder.getRemotePartsFromCompilerName(compilerName);
574+
expect(host).toEqual('godbolt.org');
575+
expect(port).toEqual(443);
576+
expect(uriBase).toEqual('gpu');
577+
578+
const {uriSchema, uri, apiPath} = CompilerFinder.prepareRemoteUrlParts(host, port, uriBase, 'c++');
579+
expect(uriSchema).toEqual('https');
580+
expect(uri).toEqual('https://godbolt.org:443/gpu');
581+
expect(apiPath).toEqual('/gpu/api/compilers/c++?fields=all');
582+
583+
const info = CompilerFinder.getRemoteInfo(uriSchema, host, port, uriBase, 'g123remote');
584+
expect(info).toEqual({
585+
target: 'https://godbolt.org:443',
586+
path: '/gpu/api/compiler/g123remote/compile',
587+
cmakePath: '/gpu/api/compiler/g123remote/cmake',
588+
basePath: '/gpu',
589+
});
590+
591+
const fullUrl = ClientOptionsHandler.getFullRemoteUrl(info);
592+
const librariesUrl = ClientOptionsHandler.getRemoteUrlForLibraries(fullUrl, 'c++');
593+
594+
expect(librariesUrl).toEqual('https://godbolt.org:443/gpu/api/libraries/c++');
595+
});
569596
});

0 commit comments

Comments
 (0)