ARMmbed / yotta

DEPRECATED: yotta build; better software
Apache License 2.0
164 stars 64 forks source link

Add $build_dir and $target_dir variables for target scripts #768

Closed marshall closed 8 years ago

marshall commented 8 years ago

This feature makes two new variables available to target scripts:

Since there were a lot of places that manually constructed the script environment and template variables, I wrote a new function called buildProgEnvAndVars to make logic like this easier to update in the future.

I also updated the unit test test/cli/test_debug.py to verify these new values.

Would love some feedback! I am using this feature in a target that I created which includes some OpenOCD configuration scripts, and I needed a reliable way to get the target's installation directory so I could pass those scripts on to OpenOCD

autopulated commented 8 years ago

This looks good, thanks!

Since path/environment related things have a tendency to be different in weird ways on windows, just want to check that you've tested this on Windows too?

One minor point of style is that in yotta:

        prog_vars = dict(program=program,
                         build_dir=build_dir,
                         target_dir=self.path)

would be better formatted as:

    prog_vars = dict(
           program = program,
         build_dir = build_dir,
        target_dir = self.path
    )

(I'm unapologetically against a literal interpretation of PEP8 here, but I think still in the same spirit ;)

Finally please could you click through the mbed contributor license agreement (https://developer.mbed.org/contributor_agreement/), and let me know the mbed username used to do that so I can go ahead and merge :)

marshall commented 8 years ago

I haven't had a chance to test this in Windows, we are running Linux / Mac only in our dev environment, so I'll get a Windows VM fired up today. Any chance that can be added to CI in the future?

One more quick question for clarification -- do you prefer review follow up as a separate commit, or do you mind if I rebase changes into the existing PR commit? :)

I signed the CLA as marshall_law

autopulated commented 8 years ago

Either works, separate commit is probably preferable.

✅ Confirmed mbed 2.0 CLA click through as marshall_law, thanks!

autopulated commented 8 years ago

I'm going to merge as-is so it can go into the next yotta release, feel free to add additional commits to polish.

marshall commented 8 years ago

Well I wasn't expecting that :) Do you guys have a public IRC or slack channel where active discussion of yotta/mbed happens?

autopulated commented 8 years ago

No problem – today was my last day at ARM, wanted to get this into a release ;)

Discussion is mostly just at http://forums.mbed.com