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

Change current directory for make process. (for nmake) #484

Merged
merged 1 commit into from
Mar 18, 2021

Conversation

elfmimi
Copy link
Contributor

@elfmimi elfmimi commented Mar 14, 2021

As far as I know, nmake does not have equivalent of -C option. So changing current directory is required.
I used subprocess.call() with cwd named argument. But it seems this is not supported in Python 2.7 .
If support for Python 2.7 is mandatory for the project , let me know. I will rework the patch.

@0xc0170
Copy link
Member

0xc0170 commented Mar 14, 2021

Hi, how to reproduce the issue to understand this patch better?

@0xc0170
Copy link
Member

0xc0170 commented Mar 14, 2021

python 2.7 would be nice but we should stop supporting it anyway soon enough. It's still enabled in Travis as being tested (I assume this specific patch for nmake is not tested in Travis well)

@elfmimi
Copy link
Contributor Author

elfmimi commented Mar 14, 2021

Ah, here you are, the context. ARMmbed/DAPLink#770 (comment)

@0xc0170
Copy link
Member

0xc0170 commented Mar 15, 2021

But it seems this is not supported in Python 2.7 .

As I read it, call in 2.7 should have args same as Popen (https://python.readthedocs.io/en/v2.7.2/library/subprocess.html#subprocess.Popen) so cwd should be supported. If that is the case, LGTM

@elfmimi
Copy link
Contributor Author

elfmimi commented Mar 15, 2021

oh yes, you are right. I tested and confirm that Python 2.7 call() do support cwd= argument. no problem then. :->

@0xc0170 0xc0170 merged commit d0bf2a7 into project-generator:master Mar 18, 2021
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