Skip to content

Shared: Add stubs for identify-environment scripts #13211

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
Merged
merged 4 commits into from
Jun 5, 2023
Merged

Shared: Add stubs for identify-environment scripts #13211

merged 4 commits into from
Jun 5, 2023

Conversation

mbg
Copy link
Member

@mbg mbg commented May 17, 2023 •
edited
Loading

The forthcoming resolve environment command in the CLI invokes these scripts. Currently only Go supports this fully and already has such a script (although it was left out of the distribution until now), while this PR adds stubs for the other languages.

@mbg mbg requested a review from owen-mc May 17, 2023 15:52
@mbg mbg self-assigned this May 17, 2023
@owen-mc
Copy link
Contributor

owen-mc commented May 18, 2023

If the Go identify-environment doesn't want to update the environment currently it outputs { "include": [] }. Should that be changed to { "configuration": { "go": {} }}? I guess this contains the information that an attempt was made to identify the environment for Go. Also the output when the environment should be updated should be changed to replace "include" by "configuration". Perhaps both of those could be done in this PR?

@mbg mbg marked this pull request as ready for review May 18, 2023 19:22
@mbg mbg requested review from a team as code owners May 18, 2023 19:22
aibaars
aibaars requested changes May 19, 2023
View reviewed changes
Copy link
Contributor

@aibaars aibaars left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think these scripts should be optional for languages that do not care about them. Same as other non-essential scripts such as pre-finalize.sh or qltest.sh. This also ensures compatibility for externally developed extractors.

If we insist on those scripts to be mandatory, I'd like the default output to be echo '{}' .

In addition I think it is a little strange that the language name is part of the JSON output. It is not really a big deal, but it feels superfluous because the scripts are per-language already.

@MathiasVP
Copy link
Contributor

I don't know anything in particular about this work, but I'd be interested in knowing how the set of languages that need this new identify-environment script was picked? For example, why don't we need one for JS, Java, C, and Python?

@aibaars
Copy link
Contributor

aibaars commented May 19, 2023

I don't know anything in particular about this work, but I'd be interested in knowing how the set of languages that need this new identify-environment script was picked? For example, why don't we need one for JS, Java, C, and Python?

I think it applies to all languages, but JS, Java, C, and Python scripts (autobuild.sh and friends) are not included in this repository.

aibaars
aibaars reviewed May 19, 2023
View reviewed changes
@owen-mc
Copy link
Contributor

owen-mc commented May 19, 2023

Are you intending to add these scripts for java?

@mbg
Copy link
Member Author

mbg commented May 19, 2023

@MathiasVP and @owen-mc: indeed as @aibaars notes, the other languages are not in this repo so there'll be a separate PR for that.

@mbg mbg force-pushed the mbg/identify-environment-stubs branch from ffb9fc9 to 3cf3549 Compare May 19, 2023 15:13
@mbg mbg removed the request for review from a team May 19, 2023 15:14
owen-mc
owen-mc previously approved these changes May 19, 2023
View reviewed changes
@mbg mbg requested review from aibaars and owen-mc May 23, 2023 09:00
aibaars
aibaars reviewed May 23, 2023
View reviewed changes
go/Makefile
@@ -14,7 +14,7 @@ CODEQL_PLATFORM = osx64
endif
endif

CODEQL_TOOLS = $(addprefix codeql-tools/,autobuild.cmd autobuild.sh pre-finalize.cmd pre-finalize.sh index.cmd index.sh tracing-config.lua)
CODEQL_TOOLS = $(addprefix codeql-tools/,autobuild.cmd autobuild.sh pre-finalize.cmd pre-finalize.sh index.cmd index.sh identify-environment.cmd identify-environment.sh tracing-config.lua)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these scripts exist? I'd expected them to be added in this PR.

Copy link
Member Author

@mbg mbg May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

They already exist, but the Makefile wasn't changed when they were added (I had mentioned this in this PR's description).

Copy link
Contributor

@owen-mc owen-mc May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

They already exist. They were added in this PR. It was an oversight that I didn't update the Makefile at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah my bad, I looked in the tools folder instead of the codeql-tools one? Is the tools folder still used in any way?

aibaars
aibaars reviewed May 23, 2023
View reviewed changes
@mbg mbg requested a review from aibaars May 23, 2023 10:10
owen-mc
owen-mc requested changes May 23, 2023
View reviewed changes
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Following recent conversations, I think the output format should be changed to remove the outer { "configuration": {"go": ... } }, and the CLI will add them in https://github.com/github/semmle-code/pull/46143.

@mbg mbg force-pushed the mbg/identify-environment-stubs branch from 03d4dc5 to 631ba65 Compare May 26, 2023 09:13
@mbg mbg requested a review from owen-mc May 26, 2023 09:15
@mbg mbg mentioned this pull request May 30, 2023
Merged
owen-mc
owen-mc previously approved these changes Jun 3, 2023
View reviewed changes
AlexDenisov
AlexDenisov reviewed Jun 5, 2023
View reviewed changes
@mbg mbg removed the request for review from aibaars June 5, 2023 08:40
aibaars
aibaars reviewed Jun 5, 2023
View reviewed changes
Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

Looks good to me.

aibaars
aibaars reviewed Jun 5, 2023
View reviewed changes
@mbg mbg merged commit 06d48dc into main Jun 5, 2023
@mbg mbg deleted the mbg/identify-environment-stubs branch June 5, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aibaars aibaars aibaars approved these changes

@henrymercer henrymercer henrymercer left review comments

@AlexDenisov AlexDenisov AlexDenisov approved these changes

@owen-mc owen-mc owen-mc approved these changes

Assignees

@mbg mbg

Labels
Go Swift
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mbg @owen-mc @MathiasVP @aibaars @AlexDenisov @henrymercer