Skip to content

Swift: db up/downgrade scripts #11205

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

Merged
merged 2 commits into from
Nov 14, 2022
Merged

Conversation

AlexDenisov
Copy link
Contributor

@AlexDenisov AlexDenisov commented Nov 10, 2022

This is inspired by the ruby checks.

The scripts check is running on macOS since the first command yields no results and breaks the whole invocation.
Perhaps, xargs works differently on macOS and Linux?

codeql resolve upgrades --format=lines --allow-downgrades --additional-packs downgrades \
         --dbscheme=ql/lib/swift.dbscheme --target-dbscheme=downgrades/initial/swift.dbscheme |
         xargs codeql execute upgrades testdb

@github-actions github-actions bot added the Swift label Nov 10, 2022
@AlexDenisov AlexDenisov force-pushed the alexdenisov/swift-db-upgrades-infra branch 3 times, most recently from 3df915d to abd11e8 Compare November 10, 2022 15:06
Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

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

This is inspired by the ruby checks.

The scripts check is running on macOS since the first command yields no results and breaks the whole invocation. Perhaps, xargs works differently on macOS and Linux?

codeql resolve upgrades --format=lines --allow-downgrades --additional-packs downgrades \
         --dbscheme=ql/lib/swift.dbscheme --target-dbscheme=downgrades/initial/swift.dbscheme |
         xargs codeql execute upgrades testdb

I think the problem is the behaviour of xargs when the input is empty (which is currently the case because we have no scripts 😅). By default on Linux the command is still run once with no input, and I'm guessing macOS does not run it at all (which by the way tbh seems the most sensible thing to do 🙂).

So either we wait to add at least one upgrade/downgrade script, or we add -r to skip xargs on empty.

@AlexDenisov
Copy link
Contributor Author

Right, from the man page:

     -r      Compatibility with GNU xargs.  The GNU version of xargs runs the utility argument at least once, even if xargs input is empty, and it supports a -r option to
             inhibit this behavior.  The FreeBSD version of xargs does not run the utility argument on empty input, but it supports the -r option for command-line
             compatibility with GNU xargs, but the -r option does nothing in the FreeBSD version of xargs.

@AlexDenisov AlexDenisov force-pushed the alexdenisov/swift-db-upgrades-infra branch from abd11e8 to 649953e Compare November 11, 2022 13:43
@AlexDenisov AlexDenisov force-pushed the alexdenisov/swift-db-upgrades-infra branch from 649953e to d49015a Compare November 11, 2022 14:51
@AlexDenisov AlexDenisov marked this pull request as ready for review November 11, 2022 14:51
@AlexDenisov AlexDenisov requested review from a team as code owners November 11, 2022 14:51
@AlexDenisov AlexDenisov requested a review from redsun82 November 11, 2022 14:51
path: swift/generated-cpp-files/**
qlformat:
Copy link
Contributor Author

@AlexDenisov AlexDenisov Nov 11, 2022

Choose a reason for hiding this comment

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

Hmm, this is most likely a rebase artifact, not sure where it comes from 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just a new line missing at the end of the file before this change. This adds a new line, so 114 changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming you also wanted to add a formatting check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is indeed a rebase artifact, the check was removed here #11214

jketema
jketema previously approved these changes Nov 11, 2022
Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

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

:shipit: But please also update the obsolete PR description 🙂

@AlexDenisov AlexDenisov merged commit d19bde8 into main Nov 14, 2022
@AlexDenisov AlexDenisov deleted the alexdenisov/swift-db-upgrades-infra branch November 14, 2022 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants