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

[cling-cpt] Added build option for the platform tags to the argument parser [skip-ci] #11020

Merged
merged 2 commits into from
Jul 28, 2022

Conversation

saisoma123
Copy link
Contributor

This Pull request: Adds another build option for building Cling with specific platform tags.

Changes or fixes: Added build options for the platform tags to the argument parser, then added additional if statements for checking whether the user wants to package it or not.

Checklist:

  • tested changes locally
  • [NA] updated the docs (if necessary)

This PR fixes #406 mentioned in (root-project/cling#406)

for this branch. This commit adds build options for the platform
tags.
@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@@ -2375,7 +2383,8 @@ def custom_input(prompt, always_yes=False):
tarball()
cleanup()

if args['tarball_tag']:
if bool(args['tarball_tag']) ^ bool(args['tarball_tag_build']):
Copy link
Member

Choose a reason for hiding this comment

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

Can ^ be replaced by or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to use an exclusive or so that two of the options couldn't be called at the same time

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to use or and make this check before we even come here. We can check and diagnose this condition where the user will know what they did wrong…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @vgvassilev just added the changes like you said

@lgtm-com
Copy link

lgtm-com bot commented Jul 25, 2022

This pull request introduces 4 alerts and fixes 10 when merging 609e0e3 into f110cf6 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 2 for Module is imported with 'import' and 'import from'

fixed alerts:

  • 10 for Module-level cyclic import

@vgvassilev vgvassilev merged commit 881b25f into root-project:master Jul 28, 2022
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.

4 participants