cnp3 / ipmininet

Mininet extension to make experimenting with IP networks easy
GNU General Public License v2.0
65 stars 51 forks source link

Pins dependency version and fix mininet install with pip #41

Closed jadinm closed 5 years ago

jadinm commented 5 years ago

Closes Issue #39

This pins versions of dependencies in the setup.py. It also installs the Mininet dependencies if not already present.

This implies changes in the install scripts: they are moved inside the ipmininet library so that they can be used by the setup.py file. The documentation is also updated.

Some points to discuss:

  1. The chosen versions for each dependency
  2. The Mininet dependencies (that are not packages) are installed in $HOME/mininet_dependencies. Maybe another location would be more suitable.
  3. The syntax mininet @ git+https://github.com/mininet/mininet@2.3.0d6 is only supported from pip 19. There is another way of installing a dependency from github but it is deprecated and it requires extra arguments when running pip. Still, this might not be the best solution.
oliviertilmans commented 5 years ago
1. The chosen versions for each dependency

Special care needs to be taken with pinned deps (i.e., '==') so that ipmininet does not prevent those packages from being upgraded in the future...

2. The Mininet dependencies (that are not packages) are installed in $HOME/mininet_dependencies. Maybe another location would be more suitable.

I'd personally for for /opt for that, and then either symlinking the exec in /usr/bin or modifying $PATH (possibly using /etc/environment or ...)

3. The syntax `mininet @ git+https://github.com/mininet/mininet@2.3.0d6` is only supported from pip 19. There is another way of installing a dependency from github but it is [deprecated](https://github.com/pypa/pip/issues/4187) and it requires extra arguments when running pip. Still, this might not be the best solution.

I am certain I used a similar syntax without issues a few years ago (possibly with the now deprecated hook). An alternative would be to auto-detect whether mininet is installed in your install script and do it 'manually' (i.e., side-stepping pip as we're not using pypi for that dep. anyway).

jadinm commented 5 years ago
1. The chosen versions for each dependency

Special care needs to be taken with pinned deps (i.e., '==') so that ipmininet does not prevent those packages from being upgraded in the future...

I only set mininet with a strict version. I chose 2.3.0d6 because 2.2.2 (the last stable version) does not support Python3.

2. The Mininet dependencies (that are not packages) are installed in $HOME/mininet_dependencies. Maybe another location would be more suitable.

I'd personally for for /opt for that, and then either symlinking the exec in /usr/bin or modifying $PATH (possibly using /etc/environment or ...)

Ok

3. The syntax `mininet @ git+https://github.com/mininet/mininet@2.3.0d6` is only supported from pip 19. There is another way of installing a dependency from github but it is [deprecated](https://github.com/pypa/pip/issues/4187) and it requires extra arguments when running pip. Still, this might not be the best solution.

I am certain I used a similar syntax without issues a few years ago (possibly with the now deprecated hook). An alternative would be to auto-detect whether mininet is installed in your install script and do it 'manually' (i.e., side-stepping pip as we're not using pypi for that dep. anyway).

Yes, with the deprecated argument, you could set a dependency_links parameter with github links from pip 10. And actually, the git link directly in the install_require parameter works since pip 18.1.

I would rather not use the manual strategy. This might prevent someone from using specific parameters of pip (e.g., "--target \<dir>") and it is also a bit strange not to have mininet as explicit python dependence.

oliviertilmans commented 5 years ago

I only set mininet with a strict version. I chose 2.3.0d6 because 2.2.2 (the last stable version) does not support Python3.

Sounds reasonable, I was mostly pointing to dependencies such as futures/...

Yes, with the deprecated argument, you could set a dependency_links parameter with github links from pip 10. And actually, the git link directly in the install_require parameter works since pip 18.1.

I would rather not use the manual strategy. This might prevent someone from using specific parameters of pip (e.g., "--target

") and it is also a bit strange not to have mininet as explicit python dependence.

Good point. I can you add version check in the setup script to warn that it will fail to install mininet and maybe print a manual fallback to be executed by the user? Beside updating pip itsel ofc (which might be managed by the distribution hence lag behind a few versions). Such issue is temporary anyway and we may just ignore it altogether past documentation.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/cnp3/ipmininet/pull/41?email_source=notifications&em ail_token=AAJVCUKIRLZ5XTY4CX7ZDALQCBRBRA5CNFSM4IHDS5U2YY3PNVW WK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3 EPATA#issuecomment-516485196 , or mute the thread https://github.com/notifications/unsubscribe- auth/AAJVCUKV5WJX4ENWRI2EYATQCBRBRANCNFSM4IHDS5UQ . https://github.com/notifications/beacon/AAJVCUNMO4N67QDCN5JF3SDQCB RBRA5CNFSM4IHDS5U2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMV XHJKTDN5WW2ZLOORPWSZGOD3EPATA.gif

jadinm commented 5 years ago

Good point. I can you add version check in the setup script to warn that it will fail to install mininet and maybe print a manual fallback to be executed by the user? Beside updating pip itsel ofc (which might be managed by the distribution hence lag behind a few versions). Such issue is temporary anyway and we may just ignore it altogether past documentation.

Ok, I modified the setup.py file to take into account the pip version. I cannot check that users added the "--process-dependency-links" parameter because it is handled internally by pip and I don't have access to that. So, I printed a warning (see below) but pip won't show it unless users add a "-v" to the pip call.

Nevertheless, this improves the PR since it is does no longer require to update pip.

# Get back Pip version
try:
    version = parse_version(require("pip")[0].version)
except IndexError:
    version = parse_version("0")
    print("We cannot find the version of pip."
          "We assume that is it inferior to 18.1.")

if version >= parse_version("18.1"):
    install_requires.append('mininet @ git+https://github.com/mininet/mininet@{ver}'
                            .format(ver=MININET_VERSION))
else:
    print("You should run pip with --process-dependency-links to install all dependencies")
    install_requires.append('mininet=={ver}'.format(ver=MININET_VERSION))
    dependency_links.append('git+https://github.com/mininet/mininet@{ver}#egg=mininet-{ver}'
                            .format(ver=MININET_VERSION))