The Wayback Machine - http://web.archive.org/web/20211124220227/https://github.com/github/codeql/pull/7166
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

Jump to bottom

Move upgrades into standard library packs #7166

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Move upgrades into standard library packs #7166

wants to merge 3 commits into from

Conversation

@dbartol
Copy link
Contributor

@dbartol dbartol commented Nov 18, 2021 •
edited
Loading

This PR moves the upgrade scripts for C#, C++, Java, JavaScript, and Python into the corresponding standard library packs (the same pack that contains the dbscheme).

Having upgrades in a separate pack gives us little or no advantage, and increases the overhead of package management. We wind up with the standard library (say, codeql/cpp-all) depending on another library pack (codeql/cpp-upgrades), but the version number of that dependency always needs to be kept in sync with the version number of the upgrade pack. In addition, no other pack for that language would have any reason to declare a dependency on the upgrade pack, so no query pack would ever have a reason to depend on a version of the upgrade pack other than the one that was published at the same time as that version of the library pack. By simply merging the packs we ensure that the right upgrade scripts are always available without having to keep two package versions in sync.

The actual changes were as follows:

  • Moved contents of <lang>/upgrades to <lang>/ql/lib/upgrades verbatim.
  • Added upgrades: upgrades property to qlpack.yml for each standard library pack.
  • Deleted qlpack*.yml for upgrade packs.
  • Removed from each standard library pack the declared dependency on the now-deleted upgrade pack.

The corresponding internal PR fixes up the build system to handle the change.

Note that Ruby already put its upgrades in its standard library.

I have not yet applied the same change to Go. I'll do that in a follow-up PR to avoid having three PRs that need to be merged simultaneously.
@dbartol dbartol requested review from aeisenberg and cklin Nov 18, 2021
@dbartol dbartol marked this pull request as ready for review Nov 18, 2021
@dbartol dbartol requested review from as code owners Nov 18, 2021
@aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Nov 18, 2021

This looks fine by me, though I think we need to get some eyes from the languages team on this before merging.

It would be very nice if we could avoid having the language server try to reconcile any .ql files in upgrades folders since they will fail compilation anyway. This was always the case, but because upgrades were in a separate pack before, it was less tempting to click on them. Not sure the best way we can implement this.

Loading

@asgerf
Copy link
Contributor

@asgerf asgerf commented Nov 19, 2021

It would be very nice if we could avoid having the language server try to reconcile any .ql files in upgrades folders since they will fail compilation anyway. This was always the case, but because upgrades were in a separate pack before, it was less tempting to click on them. Not sure the best way we can implement this.

@aeisenberg Could the language server simply treat upgrade.properties as a qlpack file with the contents:

database: old.dbscheme

Loading

@dbartol
Copy link
Contributor Author

@dbartol dbartol commented Nov 19, 2021

Does the language server issue only happen if the user opens a .ql file from an upgrade, or does it cause a problem if the user opens any file in the pack that contains upgrades?

Loading

@aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Nov 19, 2021

It only happens in the specific file you open.

Loading

@aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Nov 19, 2021

@asgerf

Could the language server simply treat upgrade.properties as a qlpack file with the contents:

I'm sure it's possible to do this. I'm not sure how much effort would be involved or if it would be worth it since these scripts are usually write-only. From your perspective how often do you find that you have to interact with upgrade scripts?

Loading

@dbartol
Copy link
Contributor Author

@dbartol dbartol commented Nov 19, 2021

If the issue only happens when opening an upgrade file, I'd prefer that it not block this PR.

Loading

@aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Nov 19, 2021

It will have to be implemented separately. This requires a change to the language server.

Loading

@dbartol
Copy link
Contributor Author

@dbartol dbartol commented Nov 20, 2021

It will have to be implemented separately. This requires a change to the language server.

Right, I meant "not block this PR or the semmle-code PR that goes with it, the latter in which we could conceivably change the language server to fix the issue."

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeisenberg aeisenberg

@cklin cklin

@github/codeql-c-analysis codeql-c-analysis

@github/codeql-csharp codeql-csharp

@github/codeql-java codeql-java

@github/codeql-javascript codeql-javascript

@github/codeql-python codeql-python

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
C# C++ depends on internal PR Java JS Python
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@dbartol @aeisenberg @asgerf