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

Bypass git dependency in build #1664

Merged
merged 3 commits into from Feb 4, 2019

Conversation

@zfields
Copy link
Contributor

commented Jan 2, 2019

Problem

git should not be a build dependency.

It is possible for the firmware to build correctly without having git installed. Currently, git is used to test which branch is being built and offer a warning message if you are working on the develop branch, without the having specified the PARTICLE_DEVELOP environment variable.

The branch check forces a dependency on Git and spews error messages in its absence. The false positive error messages obfuscate true errors and warnings, which make it harder for a developer to monitor and catch warnings and errors.

Solution

Gate the logic in checks.mk behind a check to see if git is installed

Steps to Test

Build the firmware

References

N/A


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added to CHANGELOG.md after merging (add links to docs and issues)
@kennethlimcp

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

git is also used to pull submodules for the eventual codebase with mesh firmware so removing the dependency check might end up tripping many.

@zfields

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

Git is and will always be an important part of our workflow, but "compiling the code" is a separate task from "acquiring the code" and this change will allow us to better separate our concerns.

An immediate benefit that can be realized by separating concerns is the ability to minimize the space requirements of a container housing the build dependencies (which so happens to be the motivation for this PR).

@sergeuz sergeuz self-requested a review Jan 10, 2019

@zfields

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

After a conversation with @m-mcgowan and @sergeuz, we have decided on an alternative approach.

First, we will perform a simple check to see if git is installed on the host machine. If not, the check will not be performed and no warning will be generated. Second, this check will be performed upon the conclusion of the build (as opposed to the beginning) where the warning will be displayed.

The reasoning behind this approach is as follows:

  • A warning of this nature should not block the build
  • The warning will get lost in the build spew if it is performed early on and the build is successful

@zfields zfields force-pushed the zfields:rm-dep branch from b8554e6 to d20097e Jan 11, 2019

@zfields

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

This PR now reflects part 1 (checking for git) of the complete solution described above.

@zfields zfields changed the title Remove git dependency from build Bypass git dependency from build Jan 11, 2019

@zfields zfields changed the title Bypass git dependency from build Bypass git dependency in build Jan 11, 2019

@zfields

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

Need to confirm on all platforms prior to merging...

  • Linux
  • OSX
  • Windows
@avtolstoy avtolstoy referenced this pull request Jan 23, 2019
2 of 6 tasks complete

@zfields zfields force-pushed the zfields:rm-dep branch from d20097e to 42b933e Jan 31, 2019

@@ -1,4 +1,4 @@
ifneq (, $(shell /bin/bash -c "command -v git"))
ifneq (, $(shell sh -c "command -v git"))

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Jan 31, 2019

Member

Why not simply $(shell command -v git)?

This comment has been minimized.

Copy link
@zfields

zfields Jan 31, 2019

Author Contributor

That was my first effort, it returns the following errors...

make: command: Command not found
make[1]: command: Command not found
make[2]: command: Command not found
...

command is a "built-in-utility" of a POSIX compliant shell (2.9.1 Simple Commands). It appears the default $(shell <args>) in make does not support this functionality. In order for it to work correctly, it requires modifying the SHELL variable in the Makefile. I am afraid of collateral damage, but I can avoid it completely by invoking the system shell.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Jan 31, 2019

Member

$(shell command -v git 2> /dev/null) ? Ah, but that might not be portable on Win, right?

This comment has been minimized.

Copy link
@zfields

zfields Jan 31, 2019

Author Contributor

That's not actually doing anything, it's just failing. It will fail even if git is available, because it's not finding command. That would be the same as just removing the check altogether.

This comment has been minimized.

Copy link
@zfields

zfields Jan 31, 2019

Author Contributor

Also, I didn't expect you to respond so quickly, and I updated my previous response to make it more clear.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Jan 31, 2019

Member

That's weird. command should be supported everywhere. But if it doesn't work for you, it might not work for somebody else, so sh -c command sounds safer. 👍

This comment has been minimized.

Copy link
@zfields

zfields Jan 31, 2019

Author Contributor

Thanks @avtolstoy for sticking to your guns! I learned something new, thanks to the link you shared with me about makefile shell execution.
I have updated the script according to your feedback, it looks much cleaner! Thanks for hanging with me. 😄

@zfields

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

@sergeuz Thank you for offering to test this on Mac! Please check off the box if it works successfully or drop feedback here. Cheers! Z

@zfields

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

I have confirmed the fix works for Windows!

@sergeuz

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

Works on macOS as well

@zfields

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

What else needs to be done before we merge this?

@zfields zfields removed the do not merge label Feb 4, 2019

@technobly

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

Rebase and merge :)

@zfields zfields force-pushed the zfields:rm-dep branch from 7dfc605 to 3eb275e Feb 4, 2019

@technobly technobly added this to the 1.0.1-rc.1 milestone Feb 4, 2019

@technobly technobly added the internal label Feb 4, 2019

@technobly technobly removed the enhancement label Feb 4, 2019

@technobly technobly merged commit 31d43a7 into particle-iot:develop Feb 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.