Skip to content

Conversation

@aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Dec 10, 2021

Continuation of #7166. This time it is not in a fork.

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.

@aeisenberg
Copy link
Contributor Author

Failing C# tests. Should be working when run against the PR. Here is an attempt at running it:

https://github.com/github/semmle-code/actions/runs/1565148791

@aeisenberg aeisenberg force-pushed the aeisenberg/remove-upgrades branch 2 times, most recently from 2037781 to 2b67876 Compare December 14, 2021 04:36
Move upgrade to new location

Remove incorrectly merged files

Fix upgrades section
@aeisenberg aeisenberg force-pushed the aeisenberg/remove-upgrades branch from 2b67876 to 83ceb82 Compare January 4, 2022 19:30
@aeisenberg aeisenberg requested a review from dbartol January 5, 2022 20:16
Copy link

@dbartol dbartol left a comment

Choose a reason for hiding this comment

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

LGTM

@aeisenberg aeisenberg merged commit da4f1d8 into main Jan 11, 2022
@aeisenberg aeisenberg deleted the aeisenberg/remove-upgrades branch January 11, 2022 22:09
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.

3 participants