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

Remove "run_tests.py" from data files #1534

Merged
merged 2 commits into from
Jan 8, 2021
Merged

Conversation

RobertRosca
Copy link
Contributor

"run_tests.py" is marked as a required data file for the package, but the file is now missing (seems to have been recently removed?).

So now running pip install on the repository throws an error:

  running install_data
  creating build/bdist.linux-x86_64/wheel/dials-0.0.1.data
  creating build/bdist.linux-x86_64/wheel/dials-0.0.1.data/data
  creating build/bdist.linux-x86_64/wheel/dials-0.0.1.data/data/dials
  copying conftest.py -> build/bdist.linux-x86_64/wheel/dials-0.0.1.data/data/dials
  copying __init__.py -> build/bdist.linux-x86_64/wheel/dials-0.0.1.data/data/dials
  copying libtbx_refresh.py -> build/bdist.linux-x86_64/wheel/dials-0.0.1.data/data/dials
  error: can't copy 'run_tests.py': doesn't exist or not a regular file
  ----------------------------------------
  ERROR: Failed building wheel for dials
Failed to build dials
ERROR: Could not build wheels for dials which use PEP 517 and cannot be installed directly

Removing the reference to "run_tests.py" fixes the error.

@Anthchirp
Copy link
Member

Thank you for your contribution. You are correct, and this is an issue - however, a word of warning: DIALS isn't a regular python project and we don't support the installation via setup.py/pip at this time, part of the reason for this is that right now the entire package layout is not conducive to installing DIALS as a normal python package.

For now I'd recommend following the installation instructions in https://dials.github.io/documentation/installation_developer.html instead

@codecov
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #1534 (564da0e) into master (5d21188) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1534   +/-   ##
=======================================
  Coverage   65.72%   65.72%           
=======================================
  Files         615      615           
  Lines       69122    69122           
  Branches     9550     9550           
=======================================
  Hits        45429    45429           
  Misses      21854    21854           
  Partials     1839     1839           

@ndevenish
Copy link
Member

I wonder if we should put some special code that fails setup.py installation if someone is trying this. Like, detecting the presence of the environment variable YES_I_KNOW_THIS_IS_BROKEN or something.

@RobertRosca
Copy link
Contributor Author

RobertRosca commented Jan 7, 2021

Thank you for your contribution. You are correct, and this is an issue - however, a word of warning: DIALS isn't a regular python project and we don't support the installation via setup.py/pip at this time, part of the reason for this is that right now the entire package layout is not conducive to installing DIALS as a normal python package.

For now I'd recommend following the installation instructions in https://dials.github.io/documentation/installation_developer.html instead

Aha, makes sense, thanks! I'm working with a student who's helping us at European XFEL to create Spack packages for some of the software we use, as we're planning on using Spack to manage our software installations at EuXFEL. He for help with some mysterious Failed building wheel for dials error and I didn't look into it much past noticing the missing data files, my bad.

I wonder if we should put some special code that fails setup.py installation if someone is trying this. Like, detecting the presence of the environment variable YES_I_KNOW_THIS_IS_BROKEN or something.

Something like that should be possible, glanced through the bootstrap script and it's not too clear to me if/when setup.py gets used. It's possible to add something like this to the setup.py file:

if __name__ == "__main__":
    raise Exception("manual use of setup.py is not supported")

And I think that during pip install, setup.py gets called with ['-c', 'egg_info'], so a more reliable check might be something like:

if __name__ == "__main__":
    if 'egg_info' in sys.argv:
        raise Exception("manual use of setup.py is not supported")

Either of those would throw:

    File "setup.py", line 52, in <module>
      raise Exception("manual use of setup.py is not supported")
  Exception: manual use of setup.py is not supported

Although I'm completely clueless as to how dials gets set up so maybe this is a stupid suggestion 😄

@ndevenish
Copy link
Member

My recollection was that the setup.py was used for configuring entrypoints, but it looks like that's done elsewhere (https://github.com/dials/dials/blob/master/libtbx_refresh.py is the configuration inside the libtbx build).

I imagine this was added as some initial explorations into trying to get it to build like a normal package. I don't know whether it's worth keeping this in at all.

We've started planning towards being more of a normal package as part of moving towards having a conda install dials, but it's a way off and there are other problems to solve.

Copy link
Member

@Anthchirp Anthchirp left a comment

Choose a reason for hiding this comment

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

Well, fixing up the setup.py is on the cards hopefully this year, following a pattern based on what we did with xia2. Until then I'm happy to merge this either way.

@Anthchirp Anthchirp merged commit 16cc591 into dials:master Jan 8, 2021
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.

None yet

3 participants