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

Replace automake toolchain with setuptools #1744

Closed
wants to merge 34 commits into from

Conversation

lunkwill42
Copy link
Member

@lunkwill42 lunkwill42 commented Jul 20, 2018

This PR sets out to replace the entire ancient automake-based toolchain for building and installing NAV, with a setup.py using Python's setuptools.

Some distinct features:

  • Since NAV is now all Python-code, and provides every package below the nav namespace, /python/nav was moved to /nav
  • To comply with idiomatic setuptools use, I've considered static files and sql schema files to be package data
    • Static files (/htdocs/static) have been moved to /nav/web/static
    • SASS source files have been moved from /htdocs/sass to /nav/web/sass
    • Django templates have moved from /templates to nav/web/templates
    • SQL schema data has moved from /sql/ to /nav/models/sql
    • (this would also allow templates and static files to be further dispersed into more corresponding subpackages, if necessary)
  • SASS files are now compiled using libsass, thus removing the requirement to use Ruby as part of the build tool chain.

However, I am having various issues with libsass' integration with distutils. It doesn't drop the .scss extension from the Sass files, it only appends .css to the original name, which breaks our web. There also seems to be an issue with how libsass appends the compiled css files to the distutils manifest, causing installation to fail when building and installing from an sdist tarball (such as under tox).

This means the test suite will fail to run atm. We can either work on patching libsass (for example, see sass/libsass-python#215), or we can forego the attempt and revert to keep integrating a Ruby-based sass compiler in our toolchain.

This PR is also partly motivated by dependency hell on Debian for the upcoming 4.9 release, which prompted me to once again research the option to use https://github.com/spotify/dh-virtualenv to include all of NAV's Python dependencies directly in the Debian package. It seems dh_virtualenv requires our package to provide a setup.py for installation - and so I went down the rabbit's hole....

I humbly leave this for review, while I'm off to EuroPython and 3 subsequent vacation weeks...

TODO:
* handle installation of configuration files
* handle installation of documentation
* ensure NAV knows how to get at its SQL initialization files.
* ensure tox config can handle new build system
* remove old build system
* and probably more...
we only need to assume PATH is set correctly for us.
Also, don't need sass gem now that we use libsass.
A.K.A "Why do we need to fiddle around with shell scripts to do this?".

Put daemon start/stop config into a yaml config file, and have the
nav.startstop module use that instead, entirely bypassing the ancient shell
scripts.
The `validoptions` attribute is never used, so why check it?

All of the functionality has been replaced by the nav.config module, so
this could be further reduced by removing the unnecessary use of
singleton classes (adding memoized functions or something, instead)
Let the user decide, at runtime, instead of being part of the build config,
where pid files and log files should be stored by default, and which user
daemons should be running as.
and mainly have them let nav.daemon decide where to put their
pidfiles (i.e. according to PID_DIR from nav.conf).
@lunkwill42 lunkwill42 added this to the 4.9.0 milestone Jul 20, 2018
@lunkwill42 lunkwill42 self-assigned this Jul 20, 2018
@lunkwill42 lunkwill42 requested review from jmbredal and hmpf July 20, 2018 14:03
Copy link
Contributor

@hmpf hmpf left a comment

Choose a reason for hiding this comment

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

This enormo-PR seems to be mixing up moving from automake to setuptools and refactoring how to read the config. I'm not sure it'll be worth the hassle to split it into two PRs, one for config and one for setuptools, then have the latter depend on the former, but it would satisfy my paranoia and probably be easier to test (and certainly be easier to review!).

@@ -0,0 +1,5 @@
# setuptool_scm takes care of including everything committed to git, while
Copy link
Contributor

@hmpf hmpf Jul 30, 2018

Choose a reason for hiding this comment

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

Minimal MANIFEST.in good, yes.

How have you checked that you get the files you expect? When tuning MANIFEST.in/setup.py/setup.cfg, I usually do a fake build and diff the dir-listing in both the wheel (zip) and tarball. I keep around a dir-listing of the previous version for this reason.

Would it be worth it to have a build-test?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've only made rudimentary checks. In working more with this last week, including sending some upstream PRs to the libsass-python project, it seems the MANIFEST.in may be wholly unnecessary.

We'll see as we attempt to split up this into multiple PRs.

] + build.sub_commands


setup(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is old-style setup.py. New-style is moving all this to setup.cfg and just have

from setuptools import setup

setup()

in the setup.py.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aye, but how does that work when we need to customize parts of the build with our own code?

@lunkwill42
Copy link
Member Author

I'm going to close this PR and rework it.

@lunkwill42 lunkwill42 closed this Aug 29, 2018
@lunkwill42 lunkwill42 changed the title WIP: Replace automake toolchain with setuptools Replace automake toolchain with setuptools Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants