Skip to content

Conversation

@edoardopirovano
Copy link
Contributor

@edoardopirovano edoardopirovano commented Oct 7, 2021

Short description of changes

This PR changes the building script to pass the --preserve-env option to debuild. This ensures the environment variables needed by CodeQL during the build are present.

Context: Fixes an issue?

Fixes: github/codeql-action#760

Does this change need documentation? What needs to be documented and how?

It does not - no changes to the project itself, only to the build script used in CI.

Status of this Pull Request

Working implementation.

What is missing until this pull request can be merged?

Ready to merge.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@pljones
Copy link
Collaborator

pljones commented Oct 7, 2021

Seems to fix the Linux build. Is --preserve-env safe? Should we have more controls in place along with this change?

@edoardopirovano
Copy link
Contributor Author

Seems to fix the Linux build. Is --preserve-env safe? Should we have more controls in place along with this change?

In my opinion, this is relatively safe because the build is happening in a fresh GitHub Actions environment where you know all the environment variables that have been set because there is relatively little happening before the build.

If you aren't comfortable with preserving all of the environment, we could replace this with a few instances of --preserve-envvar <varname> in order to preserve just the variables that CodeQL needs. In particular, I think --preserve-envvar "LD_PRELOAD" --preserve-envvar "ODASA_TRACER_CONFIGURATION" --preserve-envvar "CODEQL_*" would do the trick.

However, I think this is less neat and also more subject to future breakage if we change the set of environment variables we use (although I expect the CODEQL_* rule will catch everything we might add in future).

Copy link
Member

@softins softins left a comment

Choose a reason for hiding this comment

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

I'm happy to accept this PR as-is. The package building script is not used by people who are just compiling their own copies of Jamulus.

@softins softins requested a review from pljones October 7, 2021 12:24
Copy link
Collaborator

@pljones pljones left a comment

Choose a reason for hiding this comment

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

Accepting as is.

@pljones pljones merged commit 3439def into jamulussoftware:master Oct 7, 2021
@pljones
Copy link
Collaborator

pljones commented Oct 7, 2021

Oh, I should have asked @edoardopirovano to provide changes and contributors updates, too.

@edoardopirovano
Copy link
Contributor Author

Oh, I should have asked @edoardopirovano to provide changes and contributors updates, too.

Sorry, I'm not sure what you mean by that?

@ann0see
Copy link
Member

ann0see commented Oct 11, 2021

Oh, I should have asked @edoardopirovano to provide changes and contributors updates, too.

Sorry, I'm not sure what you mean by that?

You should be added as contributor here:

"<p>Gary Wang (<a href=\"https://github.com/BLumia\">BLumia</a>)</p>"

Not sure about the "changes" part. Probably the ChangeLog?

@pljones pljones added this to the Release 3.8.1 milestone Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CodeQL 1.0.16 fails on Linux (1.0.15 ok)

4 participants