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

RF: more of common logistics into setup_support #3600

Merged
merged 14 commits into from Aug 20, 2019

Conversation

@yarikoptic
Copy link
Member

commented Aug 13, 2019

I thought to provide some face lifting to our template (datalad/datalad-extension-template#4) before starting to use it, but noted that real-life cases (extensions) diverge from the template quite a bit and have lots of duplication in setup.py. I would advocate for us to absorb all common functionality within setup_support.py which we already copy around the extensions, and provide it within the template.

Well, ideally we should probably package/release it as datalad-setup-support or alike, but I am not sure if I am ready for yet another tiny packaging endeavor, so will be ok with plain copying it around the extensions for now.

If we agree that this is the way to go, then I will

  • RF and see if anything missing the extensions
    • neuroimaging
    • container
    • crawler
    • template
    • ... not familiar with the others, so will leave it to respective developers/maintainers
@mih

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Should we drop our own version handling and use versioneer?

@yarikoptic

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Should we drop our own version handling and use versioneer?

probably. I have no experience with it yet to make an informed decision and probably could be done as independent subsequent PR since this one is primarily moving code around, but FWIW nibabel is going that way too: nipy/nibabel#786

@kyleam
Copy link
Member

left a comment

If we agree that this is the way to go

This is fine by me.

import formatters as fmt


def _path_rel2file(p):
return opj(dirname(__file__), p)


def get_version():
def get_version(name):
"""Load version of datalad from version.py without entailing any imports

This comment has been minimized.

Copy link
@kyleam

kyleam Aug 13, 2019

Member

docstring should be updated to drop hard-coded "datalad".

This comment has been minimized.

Copy link
@yarikoptic

yarikoptic Aug 13, 2019

Author Member

adjusted and pushed

@codecov

This comment has been minimized.

Copy link

commented Aug 13, 2019

Codecov Report

Merging #3600 into 0.11.x will decrease coverage by 47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.11.x   #3600       +/-   ##
==========================================
- Coverage    76.6%   29.6%   -47.01%     
==========================================
  Files         252     252               
  Lines       33700   33689       -11     
==========================================
- Hits        25817    9972    -15845     
- Misses       7883   23717    +15834
Impacted Files Coverage Δ
datalad/tests/test_protocols.py 0% <0%> (-100%) ⬇️
datalad/support/tests/test_stats.py 0% <0%> (-100%) ⬇️
datalad/tests/test_constraints.py 0% <0%> (-100%) ⬇️
datalad/support/tests/test_ansi_colors.py 0% <0%> (-100%) ⬇️
datalad/distribution/tests/test_subdataset.py 0% <0%> (-100%) ⬇️
datalad/interface/tests/test_annotate_paths.py 0% <0%> (-100%) ⬇️
datalad/cmdline/tests/__init__.py 0% <0%> (-100%) ⬇️
datalad/tests/utils_testdatasets.py 0% <0%> (-100%) ⬇️
datalad/distribution/tests/test_dataset.py 0% <0%> (-100%) ⬇️
datalad/support/tests/test_status.py 0% <0%> (-100%) ⬇️
... and 153 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ee7b18...40dac18. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Aug 13, 2019

Codecov Report

Merging #3600 into 0.11.x will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.11.x    #3600      +/-   ##
==========================================
+ Coverage   77.04%   77.07%   +0.02%     
==========================================
  Files         252      252              
  Lines       33728    33728              
==========================================
+ Hits        25985    25995      +10     
+ Misses       7743     7733      -10
Impacted Files Coverage Δ
datalad/support/json_py.py 78.37% <0%> (ø) ⬆️
datalad/support/network.py 55.17% <0%> (+0.22%) ⬆️
datalad/downloaders/tests/test_http.py 91.08% <0%> (+2.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a446b48...65eb761. Read the comment docs.

@yarikoptic

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

ok, introduced some breakage:

updating environment: 80 added, 0 changed, 0 removed
reading sources... [100%] usecases/simple_provenance_tracking                   
Warning, treated as error:
/home/travis/build/datalad/datalad/docs/source/cmdline.rst:13:toctree contains reference to nonexisting document u'generated/man/datalad'
Makefile:55: recipe for target 'html' failed
make: *** [html] Error 2
make: Leaving directory '/home/travis/build/datalad/datalad/docs'

will look into it later

@kyleam

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

ok, introduced some breakage:

The problem comes from a bit above:

  File "/home/travis/build/datalad/datalad/setup_support.py", line 111, in run
    format = cls(cmdname, ext_sections=sections, version=get_version())

So the get_version call in BuildManPage needs to be updated for the new name argument.

format = cls(cmdname, ext_sections=sections, version=get_version())

BF: provide appname (hardcoded to datalad) within BuildManPage to get…
…_version()

BuildManPage ATM hardcodes for "datalad" being the module/app we care about.
And it should be just fine even if used in extensions since we do not expect
them to provide new independent commands (I guess)
@yarikoptic

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

d'oh -- thanks! pushed the workaround in 652cd4d

@yarikoptic

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

so, with the background failures now for both appveyor and travis (3.x) not easy to say if there is no side effects but it might be that we caught all the gotchas, thanks @kyleam for the review.
I guess I could proceed to try it (copy setup_support.py over) in the extensions (when get a moment) to see if anything is missing and/or needs to be changed

setup_support.py Outdated Show resolved Hide resolved
setup_support.py Show resolved Hide resolved
README = opj(dirname(__file__), 'README.md')

ret = {}
if LooseVersion(setuptools.__version__) > '38.6.0':

This comment has been minimized.

Copy link
@effigies

effigies Aug 15, 2019

Contributor

Woops. Sorry about the sloppy suggestion.

Suggested change
if LooseVersion(setuptools.__version__) > '38.6.0':
if LooseVersion(setuptools.__version__) >= '38.6.0':

This comment has been minimized.

Copy link
@yarikoptic

yarikoptic Aug 15, 2019

Author Member

Thanks -- I just amended the last commit, will force push later

@yarikoptic yarikoptic force-pushed the yarikoptic:rf-more_setup_support branch from af143e0 to 5ec34f2 Aug 15, 2019

@yarikoptic

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

ok -- I moved both setup_support (as setup.py) and formatters under datalad_build_support/ which I made into a package (added __init__.py). Seems to work to build datalad itself and datalad-container (if entire directory just symlinked, I guess would work if copied).
With and while at that, may be it is indeed to add versioneer.py to just be over with this mess ;) might look into it in upcoming days unless someone beats me to it.

@kyleam

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

I moved both setup_support (as setup.py) and formatters under datalad_build_support/

Nice. This move would also make using git-subtree possible (along the lines of what I outline in this comment), which in my view would be a big improvement over copying the files and avoids the complication of trying to work in a build dependency.

@yarikoptic

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

oh, git-subtree! indeed. But may be it is even better to make it into a submodule? then it would get its own versioned tags/releases/etc (which is what subtree wouldn't give if I get it right)?

@yarikoptic

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

cons of having a submodule

  • anyone developing would need to run git submodule update --init datalad_build_support or alike
  • investigating diff across branches if there is any difference could be trickier -- but may be it would just push us to improve datalad diff finally (e.g. #2160)
@yarikoptic

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

BTW, I don't think versioneer can leave under datalad_build_support/ - seems need to go along side with .git/ and setup.py (doc calls those "siblings" ;) ).

@yarikoptic

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

pushed the fix (e0bd962) for version being gone -- was double quoted
(git)hopa:~datalad/datalad[rf-more_setup_support]git
$> rm -rf build/man && python setup.py build_manpage && for f in ./build/man/*.*; do man $f | tail -n 2; done 
running build_manpage                                                                         
 
datalad 0.11.6                                    2019-08-15                                       datalad(1)

datalad add 0.11.6                                2019-08-15                                   datalad add(1)

datalad add-archive-content 0.11.6                2019-08-15                   datalad add-archive-content(1)

datalad add-readme 0.11.6                         2019-08-15                            datalad add-readme(1)

datalad addurls 0.11.6                            2019-08-15                               datalad addurls(1)
troff: <standard input>:35: warning: macro 'datalad.metadata.nativetype'' not defined (possibly missing space after 'da')
troff: <standard input>:49: warning: macro 'datalad_core',' not defined (possibly missing space after 'da')

datalad aggregate-metadata 0.11.6                 2019-08-15                    datalad aggregate-metadata(1)

datalad annotate-paths 0.11.6                     2019-08-15                        datalad annotate-paths(1)

datalad check-dates 0.11.6                        2019-08-15                           datalad check-dates(1)

datalad clean 0.11.6                              2019-08-15                                 datalad clean(1)

datalad clone 0.11.6                              2019-08-15                                 datalad clone(1)

datalad create 0.11.6                             2019-08-15                                datalad create(1)
@kyleam

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

oh, git-subtree! indeed. But may be it is even better to make it into a submodule?

I strongly prefer -subtree here.

then it would get its own versioned tags/releases/etc (which is what subtree wouldn't give if I get it right)?

You'd split the datalad_build_support/ into a separate repo that would have tags/releases/etc. datalad core and extensions would pull that repo into a subtree. When we are interested in updating the subtree from the upstream repo we can do so, but otherwise we can work with the tree as we would any other subdirectory in the extension repo.

cons of having a submodule

  • anyone developing would need to run git submodule update --init datalad_build_support or alike

Yes, that's a big one. For a directory as light as datalad_build_support/, I don't see any issue with the git-subtree approach of duplicating the tree objects in datalad core as well as extension repos.

  • investigating diff across branches if there is any difference could be trickier -- but may be it would just push us to improve datalad diff finally (e.g. #2160)

Also not an issue with subtrees :]

kyleam added 2 commits Aug 15, 2019
BF(py2): datalad_build_support: Add __init__.py
Before Python 3.3, an __init__.py file is required for the package to
be found.
MV: Prefix datalad_build_support/ with an underscore
datalad_setup() filters the output of find_packages(), keeping
anything that starts with "datalad", and we don't want
datalad_build_support/ to be distributed with datalad.
@yarikoptic

This comment has been minimized.

Copy link
Member

commented on 6f700cb Aug 15, 2019

Doh... I guess I forgot to git add. Thanks!

@yarikoptic

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

Ok, subtree it is then

@yarikoptic

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

Let's merge and proceed from there.
Eventually (when we probably stop carrying about easy backports) we might beefup our build setup following @effigies steps and Michael's adoption in https://github.com/datalad/git-annex-ria-remote

(edit for posterity: https://gist.github.com/effigies/c9f4194034ee218bab1668bfd7851cfc)

@yarikoptic yarikoptic closed this Aug 20, 2019

@yarikoptic yarikoptic reopened this Aug 20, 2019

@yarikoptic yarikoptic merged commit 2870060 into datalad:0.11.x Aug 20, 2019

2 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
WIP Ready for review
Details
codecov/project 77.07% (+0.02%) compared to a446b48
Details

@yarikoptic yarikoptic deleted the yarikoptic:rf-more_setup_support branch Aug 21, 2019

yarikoptic added a commit that referenced this pull request Sep 6, 2019
Merge tag '0.11.7' into debian
0.11.7 (Sep 02, 2019) -- python2-we-still-love-you-but-...

Primarily bugfixes with some optimizations and refactorings.

 Fixes

- [addurls][]
  - now provides better handling when the URL file isn't in the
    expected format.  ([#3579][])
  - always considered a relative file for the URL file argument as
    relative to the current working directory, which goes against the
    convention used by other commands of taking relative paths as
    relative to the dataset argument.  ([#3582][])

- [run-procedure][]
  - hard coded "python" when formatting the command for non-executable
    procedures ending with ".py".  `sys.executable` is now used.
    ([#3624][])
  - failed if arguments needed more complicated quoting than simply
    surrounding the value with double quotes.  This has been resolved
    for systems that support `shlex.quote`, but note that on Windows
    values are left unquoted. ([#3626][])

- [siblings][] now displays an informative error message if a local
  path is given to `--url` but `--name` isn't specified.  ([#3555][])

- [sshrun][], the command DataLad uses for `GIT_SSH_COMMAND`, didn't
  support all the parameters that Git expects it to.  ([#3616][])

- Fixed a number of Unicode py2-compatibility issues. ([#3597][])

 Enhancements and new features

- The [annotate-paths][] helper now caches subdatasets it has seen to
  avoid unnecessary calls.  ([#3570][])

- A repeated configuration query has been dropped from the handling of
  `--proc-pre` and `--proc-post`.  ([#3576][])

- Calls to `git annex find` now use `--in=.` instead of the alias
  `--in=here` to take advantage of an optimization that git-annex (as
  of the current release, 7.20190730) applies only to the
  former. ([#3574][])

- [addurls][] now suggests close matches when the URL or file format
  contains an unknown field.  ([#3594][])

- Shared logic used in the setup.py files of Datalad and its
  extensions has been moved to modules in the _datalad_build_support/
  directory.  ([#3600][])

- Get ready for upcoming git-annex dropping support for direct mode
  ([#3631][])

* tag '0.11.7': (87 commits)
  DOC: Added an entry to changelogn on merged 3631
  ENH: finalizing changelog for 0.11.7
  TST: Update tests for a git-annex without direct mode
  TST: utils: Add decorator that skips when direct mode is unsupported
  ENH: annexrepo: Refuse to initialize in direct mode if unsupported
  ENH: annexrepo: Add check_direct_mode_support method
  BF+TST: Avoid leaking patched git-annex version
  TST+RF: test_annexrepo: Split up a test
  CHANGELOG.md: Second batch for 0.11.7
  TST: run_procedure: Mark test_spaces() as known Windows failure
  TST: run_procedure: Mark test_quoting as known windows failure
  TST: run_procedure: Test more arguments that need quoting
  BF(py2): run_procedure: Avoid encoding error in log message
  TST: add run_procedure test with spaces in file name
  TST/RF: non-hardcoded Python executable
  RF: newline at end of file
  RF: helper instead of conditional
  RF: remove superfluous imports
  BF/TST: remove quoting
  ENH: replace conditionals with helper function
  ...
yarikoptic added a commit that referenced this pull request Sep 6, 2019
Merge tag '0.11.7' into debian
0.11.7 (Sep 02, 2019) -- python2-we-still-love-you-but-...

Primarily bugfixes with some optimizations and refactorings.

 Fixes

- [addurls][]
  - now provides better handling when the URL file isn't in the
    expected format.  ([#3579][])
  - always considered a relative file for the URL file argument as
    relative to the current working directory, which goes against the
    convention used by other commands of taking relative paths as
    relative to the dataset argument.  ([#3582][])

- [run-procedure][]
  - hard coded "python" when formatting the command for non-executable
    procedures ending with ".py".  `sys.executable` is now used.
    ([#3624][])
  - failed if arguments needed more complicated quoting than simply
    surrounding the value with double quotes.  This has been resolved
    for systems that support `shlex.quote`, but note that on Windows
    values are left unquoted. ([#3626][])

- [siblings][] now displays an informative error message if a local
  path is given to `--url` but `--name` isn't specified.  ([#3555][])

- [sshrun][], the command DataLad uses for `GIT_SSH_COMMAND`, didn't
  support all the parameters that Git expects it to.  ([#3616][])

- Fixed a number of Unicode py2-compatibility issues. ([#3597][])

- [download-url][] now will create leading directories of the output path
  if they do not exist ([#3646][])

 Enhancements and new features

- The [annotate-paths][] helper now caches subdatasets it has seen to
  avoid unnecessary calls.  ([#3570][])

- A repeated configuration query has been dropped from the handling of
  `--proc-pre` and `--proc-post`.  ([#3576][])

- Calls to `git annex find` now use `--in=.` instead of the alias
  `--in=here` to take advantage of an optimization that git-annex (as
  of the current release, 7.20190730) applies only to the
  former. ([#3574][])

- [addurls][] now suggests close matches when the URL or file format
  contains an unknown field.  ([#3594][])

- Shared logic used in the setup.py files of Datalad and its
  extensions has been moved to modules in the _datalad_build_support/
  directory.  ([#3600][])

- Get ready for upcoming git-annex dropping support for direct mode
  ([#3631][])

* tag '0.11.7':
  Changelog entry for download-url paths handling
  ENH: downloaders: Ensure directories for target exist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.