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

Add custom_forge activity #223

Merged
merged 14 commits into from
Aug 26, 2020
Merged

Add custom_forge activity #223

merged 14 commits into from
Aug 26, 2020

Conversation

hadim
Copy link
Contributor

@hadim hadim commented Aug 12, 2020

Because of #217 (comment), this PR is against 0.4.3.

A new activity called custom_forge is added to allow people to update a feedstock from a custom and/or private conda-forge.

I have created a new activity instead of patching the existing conda_forge one because:

  • I needed to be able to push directly in the master branch of the upstream feedstock.

    • I know it's not necessarily a good practice but we do it internally with our private forge (we only manage a dozens of packages so fixing CI errors manually is fine).
  • Our forge uses git_url instead of url so we don't need to hash an URL.

    • I also know this is not a good practice but our forge has been setup that way.

I am also fine merging this activity with the conda_forge one if you prefer.

Here are the default settings of the activity (different from `conda_forge):

$CUSTOM_FORGE_FEEDSTOCK = None
$CUSTOM_FORGE_PROTOCOL = 'ssh'
$CUSTOM_FORGE_SOURCE_URL = None
$CUSTOM_FORGE_HASH_TYPE = 'sha256'
$CUSTOM_FORGE_PULL_REQUEST = False
$CUSTOM_FORGE_RERENDER = True
$CUSTOM_FORGE_FEEDSTOCK_ORG = 'invivoai-forge'
$CUSTOM_FORGE_FORK = False
$CUSTOM_FORGE_FORK_ORG = ''
$CUSTOM_FORGE_USE_GIT_URL = True

$CUSTOM_FORGE_PATTERNS = (
    ('meta.yaml', '  version:\\s*[A-Za-z0-9._-]+', '  version: "$VERSION"'),
    (
        'meta.yaml',
        '{% set version = ".*" %}',
        '{% set version = "$VERSION" %}',
    ),
    ('meta.yaml', '  number:.*', '  number: 0'),
)

@scopatz
Copy link
Collaborator

scopatz commented Aug 16, 2020

Hey @hadim! I am totally in favor of the custom forge idea here. It looks like you just copied a lot of the conda-forge code though. Is there any way you can split some of this out of both activites and maybe get some code reuse?

Also maybe it would be good to just have a "forge" activity from which the conda-forge and/or custom forge could be specializations of.

@hadim
Copy link
Contributor Author

hadim commented Aug 16, 2020

Do you prefer to have a single "forge" activity where a user can configure conda-forge or custom forge using configuration variables or do you prefer to have two distinct activities as it is now (with as much as possible common code refactored)?

@scopatz
Copy link
Collaborator

scopatz commented Aug 16, 2020

I prefer a single forge activity, with a subclass that specializes on conda-forge, so that existing rever.xsh files aren't broken.

@hadim
Copy link
Contributor Author

hadim commented Aug 16, 2020

Sorry, this is still not clear to me @scopatz :-D

We keep the conda_forge activity as it is in terms of user-facing API. I create a forge activity that will be the parent class of the CondaForge activity (called Forge for example). That way we end up with two activities (forge and conda_forge) + two classes + factorized code + full backward compatibility.

Is that what you mean?

@scopatz
Copy link
Collaborator

scopatz commented Aug 16, 2020

Sorry for being unclear! Yeah that is exactly what I mean. I think you get it 😉

@hadim
Copy link
Contributor Author

hadim commented Aug 16, 2020

Thanks @scopatz. Will do that in the coming days/weeks.

I have a quick question related to rever in general. What is the reason it's based on xonsh?

@scopatz
Copy link
Collaborator

scopatz commented Aug 16, 2020

There are a couple of reasons:

  1. rever needs to run a lot of subcommands
  2. configuration through the environment makes sense for something like this
  3. I wanted to write a full project in xonsh

@scopatz
Copy link
Collaborator

scopatz commented Aug 16, 2020

People should feel free to submit new modules and things in Python if that makes sense though too

@hadim
Copy link
Contributor Author

hadim commented Aug 22, 2020

This PR now uses xonsh/xonsh#3705 (installed with pip install --no-deps https://github.com/xonsh/xonsh/archive/nothreadrc.zip)

@hadim
Copy link
Contributor Author

hadim commented Aug 22, 2020

(CI fails because of the xonsh version)

@hadim
Copy link
Contributor Author

hadim commented Aug 22, 2020

I think I am confused. Do I actually need to use the xonsh PR or does https://github.com/regro/rever/pull/226/files is sufficient?

@hadim
Copy link
Contributor Author

hadim commented Aug 22, 2020

@scopatz could you review please?

The Forge activity is where all the logic is and CondaForge just call Forge._func() by specifying the name of the organization conda-forge.

Forge works well so far for me but I haven't tested CondaForge on a real repo. Also, one test related to CondaForge fails but I can't find what is wrong. Do you mind having a look?

@hadim
Copy link
Contributor Author

hadim commented Aug 22, 2020

I think I am confused. Do I actually need to use the xonsh PR or does https://github.com/regro/rever/pull/226/files is sufficient?

I confirm pip install --no-deps https://github.com/xonsh/xonsh/archive/nothreadrc.zip is required for this PR to work. Here is my env:

channels:
  - conda-forge

dependencies:
  - python ==3.7.*
  - pip

  # rever deps
  # - xonsh ==0.9.18
  # pip install --no-deps https://github.com/xonsh/xonsh/archive/nothreadrc.zip
  - lazyasd
  - ruamel.yaml
  - github3.py
  - uritemplate

  - conda-smithy

  # dev
  - black
  - pytest
  - pytest-flake8
  - pytest-cov
  - pytest-timeout
  - prompt_toolkit
  - codecov
  - flake8
  - coverage
  - bibtexparser

@hadim
Copy link
Contributor Author

hadim commented Aug 22, 2020

CI is fixed. CircleCI is red because of the test failing in tests/test_conda_forge_activity.py::test_conda_forge_activity.

I will wait for this to be reviewed.

@scopatz
Copy link
Collaborator

scopatz commented Aug 23, 2020

Thanks @hadim - this is looking pretty good. Can you please add documentation hooks in the docs/ folder for the new modules and activities?

@hadim
Copy link
Contributor Author

hadim commented Aug 23, 2020

I have added the doc and news.

I still struggle with the conda-forge test. Here is the full trace. The error is github3.exceptions.AuthenticationFailed: 401 Bad credentials. I tried to set the protocol to https without success.

$ pytest -v tests/test_conda_forge_activity.py 
/home/hadim/local/conda/envs/rever/lib/python3.7/site-packages/_pytest/compat.py:340: PytestDeprecationWarning: The TerminalReporter.writer attribute is deprecated, use TerminalReporter._tw instead at your own risk.
See https://docs.pytest.org/en/stable/deprecations.html#terminalreporter-writer for more information.
  return getattr(object, name, default)
======================================================== test session starts ========================================================
platform linux -- Python 3.7.8, pytest-6.0.1, py-1.9.0, pluggy-0.13.1 -- /home/hadim/local/conda/envs/rever/bin/python
cachedir: .pytest_cache
rootdir: /home/hadim/Code/Forge/rever, configfile: pytest.ini
plugins: flake8-1.0.6, xonsh-0.9.19, cov-2.10.1, timeout-1.4.2
collected 1 item                                                                                                                    

tests/test_conda_forge_activity.py::test_conda_forge_activity FAILED                                                          [100%]

============================================================= FAILURES ==============================================================
_____________________________________________________ test_conda_forge_activity _____________________________________________________

gitrepo = '/tmp/test_conda_forge_activity', gitecho = None

    def test_conda_forge_activity(gitrepo, gitecho):
        vcsutils.tag("0.0.1")
        env = builtins.__xonsh__.env
        recipe_dir = os.path.join(env["REVER_DIR"], "rever-feedstock", "recipe")
        meta_yaml = os.path.join(recipe_dir, "meta.yaml")
        os.makedirs(recipe_dir, exist_ok=True)
        files = [
            ("rever.xsh", REVER_XSH),
            (meta_yaml, ORIG_META_YAML),
            ("credfile", CREDFILE),
        ]
        for filename, body in files:
            with open(filename, "w") as f:
                f.write(body)
        vcsutils.track(".")
        vcsutils.commit("Some versioned files")
>       env_main(["0.1.0"])

/home/hadim/Code/Forge/rever/tests/test_conda_forge_activity.py:215: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/home/hadim/Code/Forge/rever/rever/main.xsh:246: in env_main
    run_activities(ns)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

ns = Namespace(activities=None, check=False, docker_base=False, docker_install=False, entrypoint=None, force=False, rc='rever.xsh', setup=False, undo=frozenset(), version='0.1.0')

    def run_activities(ns):
        """Actually run activities."""
        need, done = compute_activities_to_run(force=ns.force)
        for name in done:
            if ns.force:
                msg = ("{YELLOW}Re-doing activity '" + name + "' which has already been "
                       "completed!{NO_COLOR}")
            else:
                msg = ("{GREEN}Activity '" + name + "' has already been "
                       "completed!{NO_COLOR}")
            print_color(msg)
        for name in need:
            act = $DAG[name]
            act.ns = ns
            status = act()
            if not status:
>               sys.exit(1)
E               SystemExit: 1

/home/hadim/Code/Forge/rever/rever/main.xsh:149: SystemExit
------------------------------------------------------- Captured stdout setup -------------------------------------------------------
Initialized empty Git repository in /tmp/test_conda_forge_activity/.git/
[master (root-commit) 44dc790] Initial readme
 1 file changed, 1 insertion(+)
 create mode 100644 README
------------------------------------------------------- Captured stdout call --------------------------------------------------------
Would have run: tag -f 0.0.1
Would have run: add .
Would have run: commit --allow-empty -am Some versioned files
activity-start:conda_forge:starting activity conda_forge
activity-error:conda_forge:activity failed with execption:
Traceback (most recent call last):
  File "/home/hadim/Code/Forge/rever/rever/activity.xsh", line 83, in __call__
    self.func(*args, **kwargs)
  File "/home/hadim/Code/Forge/rever/rever/activities/conda_forge.xsh", line 104, in _func
    recipe_dir=None,
  File "/home/hadim/Code/Forge/rever/rever/activities/forge.xsh", line 206, in _func
    repo = gh.repository(feedstock_org, feedstock_repo_name)
  File "/home/hadim/local/conda/envs/rever/lib/python3.7/site-packages/github3/github.py", line 1981, in repository
    json = self._json(self._get(url), 200)
  File "/home/hadim/local/conda/envs/rever/lib/python3.7/site-packages/github3/models.py", line 156, in _json
    raise exceptions.error_for(response)
github3.exceptions.AuthenticationFailed: 401 Bad credentials
rewinding to Would have run: rev-parse HEAD
------------------------------------------------------- Captured stderr call --------------------------------------------------------
/home/hadim/local/conda/envs/rever/lib/python3.7/site-packages/github3/session.py:3: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3,and in 3.9 it will stop working
  from collections import Callable
/home/hadim/local/conda/envs/rever/lib/python3.7/site-packages/github3/structs.py:11: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3,and in 3.9 it will stop working
  class GitHubIterator(models.GitHubCore, collections.Iterator):
====================================================== short test summary info ======================================================
FAILED tests/test_conda_forge_activity.py::test_conda_forge_activity - SystemExit: 1
========================================================= 1 failed in 0.78s =========================================================

@hadim
Copy link
Contributor Author

hadim commented Aug 23, 2020

I think I fixed the bug!

Ready for another review @scopatz!

@scopatz
Copy link
Collaborator

scopatz commented Aug 26, 2020

This looks great, thanks

@scopatz scopatz merged commit e124b71 into regro:master Aug 26, 2020
@hadim hadim deleted the custom_forge branch August 26, 2020 16:26
@hadim
Copy link
Contributor Author

hadim commented Aug 26, 2020

Besides the unit tests, I haven't manually tested the conda forge activity. I would suggest you do it before releasing a new version.

@scopatz
Copy link
Collaborator

scopatz commented Aug 26, 2020

Yeah, releasing rever will test that?

@hadim
Copy link
Contributor Author

hadim commented Aug 26, 2020

Ahaha good one :-D

@scopatz
Copy link
Collaborator

scopatz commented Aug 26, 2020

Tried it on xonsh, seems like it works conda-forge/xonsh-feedstock#149

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.

2 participants