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

Revert 981 only setup py #1020

Merged
merged 2 commits into from
May 1, 2019
Merged

Revert 981 only setup py #1020

merged 2 commits into from
May 1, 2019

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented May 1, 2019

No description provided.

@djrtwo djrtwo requested a review from protolambda May 1, 2019 16:51
Copy link
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

If we still want to go the setup.py route, we need to make sure it:

  • installs local module requirements, in a fresh copy (make clean or fresh git clone)
  • doesn't move the requirements to the full test_libs folder. That's just a hack to avoid using local dependencies. I prefer to separate debug/util packages from the pyspec module.
  • don't trust CI for an upgrade, it caches things.

@protolambda protolambda merged commit 3562264 into dev May 1, 2019
@protolambda protolambda deleted the revert-981-only_setup_py branch May 1, 2019 17:04
@djrtwo djrtwo restored the revert-981-only_setup_py branch May 1, 2019 17:17
@hwwhww
Copy link
Contributor

hwwhww commented May 2, 2019

@djrtwo @protolambda
To make sure I understand it correctly, #981 was reverted because it was merged before fully review process? And I should open another PR for it?

@protolambda

  • installs local module requirements, in a fresh copy (make clean or fresh git clone)

Do you mean in CI? It seems another issue, not related to setup.py route, correct?

  • doesn't move the requirements to the full test_libs folder. That's just a hack to avoid using local dependencies. I prefer to separate debug/util packages from the pyspec module.

Do you mean my latest comment:
#981 (comment): IMO the better solution may be using one single setup.py for all test_libs sub packages. What do you think about it?

Continue the discussion on #981, I'm still pro-single setup.py in test_libs than what #981 was (Installing config_helpers separately).

If we using one single setup.py in test_libs, we can still make deps to:

deps = {
    'config_helpers': [
        "ruamel.yaml==0.15.87",
    ],
}
deps += {
    'pyspec': [
        "eth-utils>=1.3.0,<2",
        "eth-typing>=2.1.0,<3.0.0",
        "pycryptodome==3.7.3",
        "py_ecc>=1.6.0",
    ],
    'pyspec-test': [
        "pytest>=3.6,<3.7",
    ] + deps['config_helpers'],
    ...
}

deps += {
    'pyspec-dev': deps['pyspec'] + deps['pyspec-test']
}

And install it with pip3 install -e .[pyspec] or pip3 install -e .[pyspec-test].

I see there's more value in organizing all dependencies in one file:

  • Easier to manage the dependencies. For example, both gen_base and config_helpers are depending on ruamel.yaml==0.15.87, you can update it once in the one-single setup.py.
  • If we want to release it in pypi with spec release cycle, one-single setup.py makes releasing easier.
  • Not a hacky way. :)

don't trust CI for an upgrade, it caches things.

Right, that is a nightmare. Next time we can probably update the version: https://circleci.com/docs/2.0/caching/#clearing-cache when updating without changing checksum file.

@protolambda
Copy link
Contributor

@hwwhww Will continue discussion in #981

@protolambda protolambda mentioned this pull request May 7, 2019
@hwwhww hwwhww deleted the revert-981-only_setup_py branch May 9, 2019 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants