None of the support crates are being tidy-checked or license-checked #861

brson · 2013-09-03T22:45:41Z

These are listed as exceptions in tidy.py, but most of it is maintained by us. The licensing in particular should be validated on everything we maintain.

brson · 2013-09-03T22:47:20Z

the platform directory has a similar problem

…stcommon_js_path fix testcommon.js path

frewsxcv · 2015-08-07T14:27:27Z

Looking at the list of ignored files, this might be done

SimonSapin · 2015-08-07T15:45:35Z

A bunch of "our" code (which might be defined as non-forks under https://github.com/servo/) is in separate repositories and used through cargo, and so not visible to tidy.py. So this is not done, and now harder than before we switched to Cargo and everything was a submodule.

metajack · 2015-08-07T15:47:35Z

Can't we just put tidy in each repo with a different config? Those repos should all be in the CI system, so that would achieve the same end.

SimonSapin · 2015-08-07T15:53:44Z

Each repos has its own separate CI that could have a copy of tidy.py, but that means many copies to update when we want to add something.

metajack · 2015-08-07T15:55:15Z

Move tidy to a submodule? Download it and run it during CI? There are a few approaches to reduce that problem.

frewsxcv · 2015-08-07T15:56:00Z

An option to consider: uploading tidy to PyPi as servo_tidy and just download the latest version whenever we want to tidy

EDIT: jack beat me to it

metajack · 2015-08-07T15:58:00Z

@frewsxcv But I like Corey's more concrete version of the suggestion best.

frewsxcv · 2015-08-07T16:17:49Z

Alright. I have thought about this for a little and think I am up for the challenge. I will report back with my thoughts before implementing anything; I might be able to fix #6999 simultaneously with this.

frewsxcv · 2015-08-07T20:17:50Z

After some thinking, here is what I propose:

Virtual environments (bleh?). Upon running any mach command, Python will create/activate a virtual environment (which will live somewhere like .pyenv/ or python/.pyenv/). Then we will install/update our dependencies as per a python/requirements.txt file). This will allow us to remove the following from our tree and add them in as PyPi requirements:
- python/mozdebug
- python/mozinfo
- python/mozlog
- dependencies/*
- python/toml
This will also fix pyflakes is not being registered into flake8 #6999. Depending on whether the builders blow away the working directory after every build, we might need to add some sort of caching option or a mach parameter for specifying a Python virtualenv.
Package tidy.py. This could mean just creating python/tidy/tidy.py and python/tidy/setup.py.
Incorporate tidy.py into the other repos. There are a couple ways to do this:
1. Release it on PyPi. Unless we created a system for automating publishing the Python package whenever it changes, releasing it could be a pain.
2. Having all the other repositories just do:
```
pip install -e git+https://github.com/servo/servo.git#egg=servo_tidy&subdirectory=python/tidy
```

Let me know if anyone has any other ideas or thoughts

SimonSapin · 2015-08-07T20:30:28Z

We already have a virtualenv for web-platform-tests, maybe it could also be used for this.

frewsxcv · 2015-08-07T20:44:59Z

We already have a virtualenv for web-platform-tests, maybe it could also be used for this.

Good idea. I was about to suggest that we also move tests/wpt/harness (wptrunner) out of the tree (and make it a pip dependency), but it looks like you just made some edits to our copy in-tree :P

SimonSapin · 2015-08-07T20:59:15Z

The upstream for that is https://github.com/w3c/wptrunner , and changes made in our copy are supposed to be upstreamed. I don’t know why it’s not a submodule or installed from PyPI. But that’s kinda off topic, feel free to open another issue.

I was thinking of tests/wpt/_virtualenv (which is created when you run ./mach test-wpt), not tests/wpt/harness.

metajack · 2015-08-07T21:06:57Z

Also if that _virtualenv is going to do more duties (which is fine), it should probably move higher up the tree.

frewsxcv · 2015-08-07T21:07:05Z

I was thinking of tests/wpt/_virtualenv (which is created when you run ./mach test-wpt), not tests/wpt/harness.

Right, sounds good. The Python code to generate/activate a new virtual environment is not very much, so in case we ever want to separate WPT requirements and tidy requirements in the future (due to incompatibilities or whatever) it should not be that difficult.

I am also not opposed to moving the virtualenv path to a more generic place like python/_virtualenv

frewsxcv · 2015-08-07T21:07:16Z

And yet again, jack beat me to it

metajack · 2015-08-07T21:07:43Z

python/_virtualenv seems like a good spot.

Prior to this commit: * Our Python dependency story was a bit of a mess. We had complete Python packages (wheels and directories) living in-tree, despite not having any changes from upstream. This is particularly bad because `setup.py` never gets run on these packages which could (sometimes silently) unintended breakage. * Python virtual environments (virtualenv) were only utilized for testing web-platform tests After this commit: * A single virtualenv (`python/_virtualenv`) is activated upon *every* call to mach * A requirements file (`python/requirements.txt`) is added to describe the dependencies needed by Python modules in `python/`. The child commit immediately following this will remove all the dependencies no longer needed in-tree (for the sake of keeping this commit readable). Relevant to servo#861 Fixes servo#6999

edunham · 2016-04-12T22:12:29Z

mach now uses python/_virtualenv, thanks to the resolution of #6999.

The benefit to publishing the package from where it lives in servo tree is that we wouldn't have to change all the other repos if we eventually split it out; the drawback is that we have to manage yet another set of credentials for pypi and make sure the requisite changes get pushed.

To publish on pypi, someone would have a copy of the .pypirc that holds the pypi account's username and password, then run the commands to register and upload the package. If the "someone" in this case is the buildmaster host, we could automate the process of updating the package.

With that said, I'm fine with either pypi or direct installation. Pypi is more work now, direct installation is a bigger hassle at an indeterminate point in the future, and both require making tidy into a package.

@shinglyu, do you want to move tidy and set it up as a Python package, or shall I?

shinglyu · 2016-04-13T06:56:23Z

@edunham : I can do that. :)

shinglyu · 2016-04-13T07:08:18Z

My colleague @askeing is very interested in this issue, so I'll leave this to him.

askeing · 2016-04-13T07:40:54Z

Hi @edunham ,
I try to set up it as the Python package (with tests) here: https://github.com/askeing/servo_tidy

edunham · 2016-04-13T16:42:49Z

@askeing Thank you! It looks like you've done most of the hard work already. My only nitpick is that it'd be helpful to have setup() in setup.py include something like

entry_points={
        'console_scripts': [
            'servo-tidy=servo_tidy:scan',
        ],
    },

so that we can just invoke servo-tidy directly in the script section of another project's .travis.yml once the package has been installed.

edunham · 2016-04-13T16:53:14Z

@frewsxcv @larsbergstrom @metajack What are your thoughts on tidy living in the Servo tree vs in its own repo? How important to the project is it to keep the tidy git history from the current tree? I personally prefer to keep history whenever possible, but that's got more to do with opinion than necessity in this case.

frewsxcv · 2016-04-13T16:56:01Z

No strong preference from me. Worth mentioning that tidy.py seems to be a starting point for some new contributors, and having that file live in a separate repository might increase the barrier those contributors.

jdm · 2016-04-13T17:05:52Z

I think the most important concern is that it's possible to modify tidy.py and see how those changes affect ./mach test-tidy with the fewest possible intermediate steps.

servo#861 (comment) "I think the most important concern is that it's possible to modify tidy.py and see how those changes affect ./mach test-tidy with the fewest possible intermediate steps." - jdm This takes publishing complexity away from the contributor when testing changes and makes it an infra problem instead, where it's much easier to automate & saner to manage pypi credentials

@askeing

Package tidy This fixes #861. @askeing, I've copied your work from https://github.com/askeing/servo_tidy and attributed the commit to you. My commit in this PR is Git housekeeping to preserve `tidy`'s history. If you'd like to make additional changes, I've given you and @shinglyu push access to my fork of Servo. Apologies if this is already familiar, but the workflow for pushing to my branch is: ``` $ git remote add edunham git@github.com:edunham/servo.git $ git checkout -b package-tidy $ git pull edunham package-tidy $ git push edunham package-tidy ``` Once this lands, I'll look at how to publish it to PyPI and automate that process. Please don't merge this yet; we still need to discuss how the change should work around https://github.com/servo/servo/blob/master/python/servo/testing_commands.py#L33 , as I've yet to figure out how to get the egg to actually expose its tests.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10590)

edunham · 2016-04-15T20:06:27Z

Oops, saying "fixes" in that PR was going a bit far. The crates aren't actually using it in their CI yet.

nox · 2017-09-29T08:24:45Z

I'm pretty sure this has been down by now, or that other issues superseded this one.

…sbergstrom This fixes servo/servo#861. askeing, I've copied your work from https://github.com/askeing/servo_tidy and attributed the commit to you. My commit in this PR is Git housekeeping to preserve `tidy`'s history. If you'd like to make additional changes, I've given you and shinglyu push access to my fork of Servo. Apologies if this is already familiar, but the workflow for pushing to my branch is: ``` $ git remote add edunham gitgithub.com:edunham/servo.git $ git checkout -b package-tidy $ git pull edunham package-tidy $ git push edunham package-tidy ``` Once this lands, I'll look at how to publish it to PyPI and automate that process. Please don't merge this yet; we still need to discuss how the change should work around https://github.com/servo/servo/blob/master/python/servo/testing_commands.py#L33 , as I've yet to figure out how to get the egg to actually expose its tests. Source-Repo: https://github.com/servo/servo Source-Revision: bfe54539d290cb287e59e8ba106a54a3fab6201a UltraBlame original commit: 11cc0b9e7ced9d12abf44b25fff16081f19730f2

jdm added the A-infrastructure label Apr 18, 2014

ChrisParis pushed a commit to ChrisParis/servo that referenced this issue Sep 7, 2014

Merge pull request servo#861 from sizuhiko/submission/sizuhiko/fix_te…

7f41b3c

…stcommon_js_path fix testcommon.js path

frewsxcv added the C-is-this-done It is unclear if the issue has been resolved label Aug 7, 2015

frewsxcv removed the C-is-this-done It is unclear if the issue has been resolved label Aug 7, 2015

metajack mentioned this issue Aug 7, 2015

CI improvement meta bug #7080

Closed

7 tasks

edunham mentioned this issue Apr 13, 2016

Package tidy #10590

Merged

bors-servo closed this as completed in #10590 Apr 15, 2016

edunham reopened this Apr 15, 2016

edunham mentioned this issue Apr 15, 2016

Add tidy to dependencies' CI #10636

Closed

18 tasks

frewsxcv mentioned this issue Jul 14, 2016

Add license field to Cargo.toml files #12434

Closed

edunham mentioned this issue Jul 14, 2017

Add tidy to support crates' CI #17733

Closed

3 tasks

nox closed this as completed Sep 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

None of the support crates are being tidy-checked or license-checked #861

None of the support crates are being tidy-checked or license-checked #861

brson commented Sep 3, 2013

brson commented Sep 3, 2013

frewsxcv commented Aug 7, 2015

SimonSapin commented Aug 7, 2015

metajack commented Aug 7, 2015

SimonSapin commented Aug 7, 2015

metajack commented Aug 7, 2015

frewsxcv commented Aug 7, 2015

metajack commented Aug 7, 2015

frewsxcv commented Aug 7, 2015

frewsxcv commented Aug 7, 2015

SimonSapin commented Aug 7, 2015

frewsxcv commented Aug 7, 2015

SimonSapin commented Aug 7, 2015

metajack commented Aug 7, 2015

frewsxcv commented Aug 7, 2015

frewsxcv commented Aug 7, 2015

metajack commented Aug 7, 2015

edunham commented Apr 12, 2016

shinglyu commented Apr 13, 2016

shinglyu commented Apr 13, 2016

askeing commented Apr 13, 2016

edunham commented Apr 13, 2016

edunham commented Apr 13, 2016

frewsxcv commented Apr 13, 2016

jdm commented Apr 13, 2016

edunham commented Apr 15, 2016

nox commented Sep 29, 2017

None of the support crates are being tidy-checked or license-checked #861

None of the support crates are being tidy-checked or license-checked #861

Comments

brson commented Sep 3, 2013

brson commented Sep 3, 2013

frewsxcv commented Aug 7, 2015

SimonSapin commented Aug 7, 2015

metajack commented Aug 7, 2015

SimonSapin commented Aug 7, 2015

metajack commented Aug 7, 2015

frewsxcv commented Aug 7, 2015

metajack commented Aug 7, 2015

frewsxcv commented Aug 7, 2015

frewsxcv commented Aug 7, 2015

SimonSapin commented Aug 7, 2015

frewsxcv commented Aug 7, 2015

SimonSapin commented Aug 7, 2015

metajack commented Aug 7, 2015

frewsxcv commented Aug 7, 2015

frewsxcv commented Aug 7, 2015

metajack commented Aug 7, 2015

edunham commented Apr 12, 2016

shinglyu commented Apr 13, 2016

shinglyu commented Apr 13, 2016

askeing commented Apr 13, 2016

edunham commented Apr 13, 2016

edunham commented Apr 13, 2016

frewsxcv commented Apr 13, 2016

jdm commented Apr 13, 2016

edunham commented Apr 15, 2016

nox commented Sep 29, 2017