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.

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

Local/Remote Execution environment #6413

Merged
merged 23 commits into from
May 26, 2024
Merged

Local/Remote Execution environment #6413

merged 23 commits into from
May 26, 2024

Conversation

partouf
Copy link
Contributor

@partouf partouf commented Apr 29, 2024

First working version

This is part 1 of many for #6379

Changed in this PR:

  • Move User Executable Execution into it's own independent thing
  • Stored some extra things in buildResult so execution can work without knowledge of the compiler
  • Be able to use a Hash and some JSON to execute

This so that later:

Will comment out the API Endpoint before merging of course

lib/execution-env.ts Fixed Show resolved Hide resolved
lib/execution/base-execution-env.ts Dismissed Show dismissed Hide dismissed
lib/handlers/api.ts Dismissed Show dismissed Hide dismissed
lib/handlers/api.ts Dismissed Show dismissed Hide dismissed
@github-actions github-actions bot added the lang-c label May 4, 2024
@partouf partouf marked this pull request as ready for review May 4, 2024 20:15
@partouf
Copy link
Contributor Author

partouf commented May 4, 2024

Took more refactoring then I had hoped, but I'm happy with the end-result

@partouf
Copy link
Contributor Author

partouf commented May 4, 2024

How I tested this:

  1. Build executable with regular CE with debug on
  2. Copy paste the hash for the cached executable (123abc_exec hash)
  3. Use the hash on script below:
#!/bin/sh

get_post_json() {
	cat <<'EOF'
{
	"ExecutionParams": {
		"args": ["Hello", "args!"],
		"stdin": "Hello stdin!",
		"runtimeTools": [
			{
				"name": "env",
				"options": [
					{
					"name": "PETE",
					"value": "Hello123"
					}
				]
			},
			{
				"name": "heaptrack",
				"options": [
					{
						"name": "details",
						"value": "stderr"
					},
					{
						"name": "graph",
						"value": "yes"
					}
				]
			}
		]
	}
}
EOF
}

POSTJSON=$(get_post_json)

echo $POSTJSON | jq

curl -s "http://127.0.0.1:10240/api/localexecution/$1" -H "Accept: application/json" -H "Content-type: application/json" -d "$POSTJSON" | jq

@partouf partouf requested a review from mattgodbolt May 4, 2024 20:33
@mattgodbolt mattgodbolt self-assigned this May 4, 2024
@mattgodbolt
Copy link
Member

...will comment out the API endpoint

Maybe make it a configuration option (default off, naturally).

Copy link
Member

@mattgodbolt mattgodbolt left a comment

Choose a reason for hiding this comment

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

Remarkably little change to get this I see (mostly code motion), nice work. Discussing more in person.

test/demangler-tests.ts Show resolved Hide resolved
lib/handlers/api.ts Show resolved Hide resolved
import {ExecutionParams} from '../../types/compilation/compilation.interfaces.js';
import {BasicExecutionResult, ExecutableExecutionOptions} from '../../types/execution/execution.interfaces.js';

export interface IExecutionEnvironment {
Copy link
Member

Choose a reason for hiding this comment

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

Nice

lib/compilers/dotnet.ts Show resolved Hide resolved
lib/compilation-env.ts Show resolved Hide resolved
lib/base-compiler.ts Show resolved Hide resolved
@partouf
Copy link
Contributor Author

partouf commented May 23, 2024

Breaks Heaptrack Graph in Cmake + Execution https://godbolt.org/staging/z/zz6M7TWqx
(works on prod)

@partouf
Copy link
Contributor Author

partouf commented May 23, 2024

F# execution broken https://godbolt.org/staging/z/W3nazK8oc

@partouf
Copy link
Contributor Author

partouf commented May 26, 2024

Breaks Heaptrack Graph in Cmake + Execution https://godbolt.org/staging/z/zz6M7TWqx (works on prod)

The artifacts here moved from the root of the result object to execResult

This is not super-wrong and it might be better in case of situations where you want both build and execution artifacts.

So I'll add a thing to the frontend instead of changing it back.

@github-actions github-actions bot added the ui label May 26, 2024
@partouf
Copy link
Contributor Author

partouf commented May 26, 2024

fixed the broken things

@partouf partouf merged commit d2c7a52 into main May 26, 2024
17 checks passed
@partouf partouf deleted the execution-api-test branch May 26, 2024 20:09
@dkm
Copy link
Member

dkm commented May 29, 2024

Not really sure if there's anything to test here... but still, this is live :)

@partouf
Copy link
Contributor Author

partouf commented May 29, 2024

Not really sure if there's anything to test here... but still, this is live :)

Thanks! (technically it should just not break anything :) and I've tested it well enough that I'm not expecting any issues)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants