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

Make python a required build dependency #5784

Merged
merged 3 commits into from
May 6, 2019
Merged

Make python a required build dependency #5784

merged 3 commits into from
May 6, 2019

Conversation

skullydazed
Copy link
Member

@skullydazed skullydazed commented May 4, 2019

As described in #5776 this adds python to all the OS install scripts.

Description

We need to test on the following platforms:

  • FreeBSD
  • Fedora
  • Debian/Ubuntu
  • Arch/Manjaro
  • Gentoo
  • sabayon
  • OpenSUSE/Tumbleweed
  • Slackware
  • macOS
  • msys2
  • WSL

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@skullydazed
Copy link
Member Author

skullydazed commented May 4, 2019

If you have one of the affected systems please give this a try to make sure it works.

If you don't yet have python 3 installed:

  1. Merge this PR into your tree
  2. Build a keyboard, for example make clueboard/66/rev3:default
  3. You should get a message saying Python 3 is not installed. It will be required by a future version of qmk_firmware
  4. Run util/qmk_install.sh
    • Look at the output to make sure no errors are thrown
  5. Run the make command again, the warning message should not show up

If you already have python3 installed you can skip steps 2 and 3.

@skullydazed
Copy link
Member Author

Tagging people who have contributed some of the less used platforms. If you are on this list please take a look at this PR and ensure that your OS still works and installs python3.

@manuel-arguelles @Chris-Johnston @vatho @florensie @zjp @vomindoraan @BlitzKraft @shieldsd

@BlitzKraft
Copy link
Contributor

@skullydazed For sabayon, python3 is the default python. Changing the line 109 to dev-lang/python will suffice. Recent updates dropped python2. Python2 requires explicit version pinning.

Also, the colon in the line 109 is not correct. It needs to be a hyphen, for specifying version numbers. For python3.5, the package name is dev-lang/python-3.5.5-r1.

I tested the script, and works with the corrections. Without specifying version, python3.6.6 is installed. With specifying 3.5.5-r1, the corresponding package is installed.

@manuel-arguelles
Copy link
Contributor

+1 for Slackware

Copy link
Contributor

@Chris-Johnston Chris-Johnston left a comment

Choose a reason for hiding this comment

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

Tested working on Linux Mint 18.3 and 19.1

$ cat /etc/upstream-release/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=16.04
DISTRIB_CODENAME=xenial
DISTRIB_DESCRIPTION="Ubuntu 16.04 LTS"
$ cat /etc/upstream-release/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=18.04
DISTRIB_CODENAME=bionic
DISTRIB_DESCRIPTION="Ubuntu 18.04 LTS"

using the steps provided.

@skullydazed
Copy link
Member Author

@BlitzKraft Thanks for the report. Does my latest commit fix those issues?

@BlitzKraft
Copy link
Contributor

Yes, it does fix the issues @skullydazed

@zjp
Copy link
Contributor

zjp commented May 5, 2019

It might take me a minute to check -- it's right around finals for me and every class I'm in has a project due next week. If no one has checked on Gentoo by Friday, I'll look at it.

@skullydazed
Copy link
Member Author

@zjp That's fine. If we end up merging this sooner and there is a problem with gentoo a new PR can be created to fix it up.

@yanfali
Copy link
Contributor

yanfali commented May 5, 2019

Tested it on arch linux updated today. Kernel 5.0.10.

  1. installed using ./install_linux.sh
    it re-installed stuff but added arm gcc. No errors. Python 3.7.3 is current default on arch.
  2. compiled AVR keyboard make keebio/iris:default
    No errors encountered
  3. compiled ARM keyboard make clueboard/60:default
    1 error encountered. Needed make git-submodule

LGTM

Copy link
Contributor

@yanfali yanfali left a comment

Choose a reason for hiding this comment

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

👍 for arch linux. Python 3.7.3 is the default.

@drashna drashna requested a review from ezuk May 5, 2019 23:43
Copy link
Contributor

@vomindoraan vomindoraan left a comment

Choose a reason for hiding this comment

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

A few minor things. LGTM otherwise.

@@ -534,6 +534,8 @@ endef
%:
# Check if we have the CMP tool installed
cmp $(ROOT_DIR)/Makefile $(ROOT_DIR)/Makefile >/dev/null 2>&1; if [ $$? -gt 0 ]; then printf "$(MSG_NO_CMP)"; exit 1; fi;
# Ensure that python3 is installed. This check can be removed after python is used in more places.
if ! python3 --version 1> /dev/null 2>&1; then printf "$(MSG_PYTHON_MISSING)"; fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ! python3 --version 1> /dev/null 2>&1; then printf "$(MSG_PYTHON_MISSING)"; fi
if ! python3 --version >/dev/null 2>&1; then printf "%s" "$(MSG_PYTHON_MISSING)"; fi

SC2059: Don't use variables in the printf format string. Use printf "..%s.." "$foo".
Also change the stdout redirection to be consistent with the rest of the file.

@@ -81,3 +81,7 @@ MSG_FILE_TOO_BIG = $(ERROR_COLOR)The firmware is too large!$(NO_COLOR) $(CURRENT
MSG_FILE_TOO_SMALL = The firmware is too small! $(CURRENT_SIZE)/$(MAX_SIZE)\n
MSG_FILE_JUST_RIGHT = The firmware size is fine - $(CURRENT_SIZE)/$(MAX_SIZE) ($(FREE_SIZE) bytes free)\n
MSG_FILE_NEAR_LIMIT = The firmware size is approaching the maximum - $(CURRENT_SIZE)/$(MAX_SIZE) ($(FREE_SIZE) bytes free)\n
MSG_PYTHON_MISSING = $(WARN_COLOR)WARNING:$(NO_COLOR)\n \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MSG_PYTHON_MISSING = $(WARN_COLOR)WARNING:$(NO_COLOR)\n \
MSG_PYTHON_MISSING = $(WARN_COLOR)WARNING:$(NO_COLOR)\n\

I think there's no need for the extra space.

@skullydazed skullydazed merged commit 9950024 into master May 6, 2019
@skullydazed skullydazed deleted the python_support branch May 6, 2019 17:56
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request May 7, 2019
* Make python a required build dependency

* Add missing color

* fixup sabayon linux per @BlitzKraft
KauyonKais pushed a commit to KauyonKais/qmk_firmware that referenced this pull request May 8, 2019
* Make python a required build dependency

* Add missing color

* fixup sabayon linux per @BlitzKraft
KauyonKais pushed a commit to KauyonKais/qmk_firmware that referenced this pull request May 8, 2019
* Make python a required build dependency

* Add missing color

* fixup sabayon linux per @BlitzKraft
fdidron added a commit to zsa/qmk_firmware that referenced this pull request May 20, 2019
Make python a required build dependency (qmk#5784)
Timbus pushed a commit to Timbus/qmk_firmware that referenced this pull request Jun 23, 2019
* Make python a required build dependency

* Add missing color

* fixup sabayon linux per @BlitzKraft
@zjp
Copy link
Contributor

zjp commented Jul 11, 2019

Sorry to necro this so late but I was able to get my environment set up. Per the Python page on the wiki, it's very integral to Gentoo, so it's unlikely to not be installed on the system. The default right now is Python 3.6

I'll say looks good to me, even without dev-lang/python in the install script. If, periodically, pulling the repo breaks something for me, I'll take a look at why and update.

ridingqwerty pushed a commit to ridingqwerty/qmk_firmware that referenced this pull request Jan 10, 2020
* Make python a required build dependency

* Add missing color

* fixup sabayon linux per @BlitzKraft
JeffreyPalmer pushed a commit to JeffreyPalmer/qmk_firmware that referenced this pull request Feb 27, 2020
* Make python a required build dependency

* Add missing color

* fixup sabayon linux per @BlitzKraft
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.

8 participants