Skip to content

Cleanup emcmake/emconfigure #10392

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
Feb 18, 2020
Merged

Cleanup emcmake/emconfigure #10392

merged 2 commits into from
Feb 18, 2020

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Feb 8, 2020

  • Set CMAKE_CROSSCOMPILING_EMULATOR in the same way we set
    CMAKE_TOOLCHAIN_FILE.
  • Consistent echoing of make/configure command.
  • Don't silently succeed if passed no args

@sbc100 sbc100 changed the title Clean emcmake Cleanup emcmake/emconfigure Feb 8, 2020
@sbc100 sbc100 force-pushed the emcmake_cleanup branch 2 times, most recently from 1d16c82 to 75f398f Compare February 8, 2020 17:26
@sbc100 sbc100 requested a review from kripken February 8, 2020 18:42
run_process(args, stdout=stdout, stderr=stderr, env=env, **kwargs)
if 'EMMAKEN_JUST_CONFIGURE' in env:
del env['EMMAKEN_JUST_CONFIGURE']
Copy link
Member

Choose a reason for hiding this comment

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

why don't we need to clean this up?

Copy link
Collaborator Author

@sbc100 sbc100 Feb 13, 2020

Choose a reason for hiding this comment

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

Because env is local thing.. its does not live after this function.

Copy link
Member

Choose a reason for hiding this comment

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

but env can be passed in as a parameter, in which case we modify it? (do we copy it somewhere that I missed?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I think i had an env.copy somewhere in a previous version of this change. I'll re-add that.

Copy link
Member

Choose a reason for hiding this comment

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

lgtm with that

@sbc100 sbc100 force-pushed the emcmake_cleanup branch 2 times, most recently from 358a0e7 to 434a7bb Compare February 16, 2020 03:50
- Set CMAKE_CROSSCOMPILING_EMULATOR in the same way we set
  CMAKE_TOOLCHAIN_FILE.
- Consisten echoing of make/configure command.
- Don't silently succeed if passed no args
@sbc100 sbc100 merged commit 1b3f0d0 into master Feb 18, 2020
@sbc100 sbc100 deleted the emcmake_cleanup branch February 18, 2020 22:25
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.

2 participants