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

Update python module versions in requirements.txt #8542

Merged
merged 1 commit into from Nov 1, 2018

Conversation

Projects
None yet
10 participants
@cmonr
Contributor

cmonr commented Oct 25, 2018

Description

A followup to #8390 and #8446.

Allowing packages to have no upper version bound is inviting the above problems. Instead, packages should remain fixed at a max version unless it's specifically requested that a newer version is needed.

With #8446, pip list was added into travis to get a list of exactly which package versions are being used in CI. The upadted requirements.txt reflects this list: https://travis-ci.org/cmonr/mbed-os/jobs/442420121#L458

Ideally, all packages would have a fixed version, but it seems unrealistic that users would only have the specific version. My thinking is that this would cause more problems than it solves, so the initial net is cast really wide.

Locally tested by instantiating fresh pipenv directories using 2.7.15 and 3.7.1 anbd running pip install -r ../mbed-os/requirements.txt in Arch Linux. Both instances worked without issues.

Resolves #8396

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@cmonr cmonr requested review from ARMmbed/mbed-os-maintainers Oct 25, 2018

@cmonr cmonr added the needs: review label Oct 25, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 25, 2018

@JanneKiiskila Would you mind asking someone from your team to look this over? I know that y'all have your own CI, and I'd prefer to avoid breaking that with this change if possible (it shouldn't, but I like to make sure).

@cmonr cmonr requested a review from ARMmbed/mbed-os-tools Oct 25, 2018

@cmonr cmonr changed the title from Updated all python modules to either have a max or specific version. to Updated python module versions in requirements.txt Oct 25, 2018

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Oct 25, 2018

I've asked @anttiylitokola to review this. The pr-head CI job does test the client, too - it's passing at least the build.

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Oct 25, 2018

One process question though - if you now set the upper limits,

  • who will be raising those
  • and based on what input
  • and how frequently?
@anttiylitokola

This comment has been minimized.

Contributor

anttiylitokola commented Oct 25, 2018

Looks to be working fine also in our CI.

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Oct 25, 2018

I don't understand...
I often upgrade all my python packages to the latest version...
Now, I will need to check all the module 1 by 1 ?

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Oct 25, 2018

This would fix every module to a specific version, unless someone does a PR and changes the requirements.txt. Which, in itself is fine - that makes CIs etc. very stable, as there are no changes. However, the question is - when does it get updated and by whom? On the other hand we do not want to use stale SW either, i.e. modules with known security holes etc.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 25, 2018

@jeromecoutant This was brought on because in the past two weeks, CI (and as a result master) broke because all modules were being pulled to the latest versions, and some modules were not compatible with each other (Ref: #8390, #8446).

I often upgrade all my python packages to the latest version...

I can actually guarantee that you're not using the latest versions because the two referenced PRs version-locked two modules. I think it's preferable to not have the latest version of something and have everything work, instead of the opposite.

Now, I will need to check all the module 1 by 1?

Not sure I understand the question. You can still point pip to install/update from the requirements.txt file, and it will check your installed versions with the requested package versions.

However, the question is - when does it get updated and by whom? On the other hand we do not want to use stale SW either, i.e. modules with known security holes etc.

@JanneKiiskila That's a good question. My initial thoughts is that we update and test the latest public versions somewhere in between feature releases (in line with keeping CI stable during feature release), but if someone needs a newer/older version of a tool, an issue could be opened and the module version be evaluated as a part of that issue.

Part of this PR is to get feedback on that as well.

@cmonr cmonr changed the title from Updated python module versions in requirements.txt to Update python module versions in requirements.txt Oct 25, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 29, 2018

Fwiw, setting an upper bound should also make us more resiliant against upgrade attacks, such as https://www.zdnet.com/article/twelve-malicious-python-libraries-found-and-removed-from-pypi/

The actual fix to make sure this doesn't happen to users installing requirements.txt is to add all modules needed, and version lock them, but one step at a time.

@adbridge

@cmonr how have you decided which upper version to use? Also if we have a known set of working versions would it not just be safer to lock them now to the latest known working version?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 29, 2018

@adbridge

The upadted requirements.txt reflects this list: https://travis-ci.org/cmonr/mbed-os/jobs/442420121#L458

Also if we have a known set of working versions would it not just be safer to lock them now to the latest known working version?

Safer, probably. But what are the odds that all of our CIs are also set to the same vesrions?

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 29, 2018

Actually all our CIs should be on the same versions for consistency so this would ensure that.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 29, 2018

Actually all our CIs should be on the same versions for consistency so this would ensure that.

Sorry, but the intention of this PR isn't to force everyone, including users, to only use specific versions of tools.

The intention is to make it much more unlikely that we'll be hit with the class of issues that caused the need for #8390 and #8446, with what should be minimal impact to other groups.

Yes it would be good to get all of our known CIs aligned against testing versions, but that wasn't the scope of this fix.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 30, 2018

@theotherjimmy waiting for your review

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 31, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 31, 2018

Build : SUCCESS

Build number : 3522
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8542/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 1608fbb into ARMmbed:master Nov 1, 2018

15 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 677 files (+0 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9184 cycles (-1068 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Nov 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment