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

C++/C#: Move the Windows autobuilder into a subfolder in ql/csharp. #16487

Merged
merged 3 commits into from
May 15, 2024

Conversation

criemen
Copy link
Collaborator

@criemen criemen commented May 14, 2024

This is a necessary preparation for moving the C# dependency management to paket, which in turn is a necessary preparation for moving the C# build to bazel.

As we discovered in #16376, paket tries to restore all projects recursively from the root folder. If we support building C# code under both ql/csharp and ql/cpp, we need to have a single lockfile under ql, as both codebases share the same set of dependencies (and utilities from ql/csharp/extractor).
Then, paket will also try to restore things that look like "C# projects" in other languages' folders, which is not what we want.
Therefore, we address this by moving all C# code into a common root directory, ql/csharp.

This needs an internal PR to adjust the buildsystem to look for the autobuilder in the new location.

This is a necessary preparation for moving the C# dependency management to `paket`,
which in turn is a necessary preparation for moving the C# build to bazel.

As we discovered in #16376,
`paket` tries to restore all projects recursively from the root folder.
If we support building C# code under both `ql/csharp` and `ql/cpp`, we need
to have a single lockfile under `ql`, as both codebases share the same set of dependencies
(and utilities from `ql/csharp/extractor`).
Then, `paket` will also try to restore things that look like "C# projects" in other languages'
folders, which is not what we want.
Therefore, we address this by moving all C# code into a common root directory, `ql/csharp`.

This needs an internal PR to adjust the buildsystem to look for the autobuilder in the new location.
@criemen criemen force-pushed the criemen/move-win-autobuilder branch from 18930cc to 869bf8a Compare May 14, 2024 11:46
@criemen criemen added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label May 14, 2024
@criemen criemen marked this pull request as ready for review May 14, 2024 13:09
@criemen criemen requested review from a team as code owners May 14, 2024 13:09
tamasvajk
tamasvajk previously approved these changes May 14, 2024
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

LGTM

@criemen
Copy link
Collaborator Author

criemen commented May 14, 2024

As discussed in the other PR, waiting for explicit approval from @github/codeql-c-extractor , who are currently at an offsite.

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

Change-wise this LGTM, just one little comment.

Before approving this I'd like to understand two things related to releases:

  • Will the C++ Windows autobuilder still be included?
  • Will the C++ Windows autobuilder still be included in such a way that it is correctly picked up when attempting autobuild of a C/C++ project on Windows?

CODEOWNERS Outdated Show resolved Hide resolved
Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com>
@criemen
Copy link
Collaborator Author

criemen commented May 15, 2024

Re releases: The internal PR changes the path where we're looking for the autobuilder, so that it's still correctly included in the released bundle. I verified that manually with a bundle build. The invocation of the autobuilder doesn't change at all by the moving of the directories.

@criemen criemen merged commit eb9c734 into main May 15, 2024
17 checks passed
@criemen criemen deleted the criemen/move-win-autobuilder branch May 15, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants