Skip to content

/ servo

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

Added test-tidy for dependent licenses. #12511

Merged
merged 1 commit into from Jul 21, 2016
Merged

Added test-tidy for dependent licenses. #12511

merged 1 commit into from Jul 21, 2016

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Jul 19, 2016 •
edited by larsbergstrom

Add a test-tidy lint for dependency licenses. Note that #12507 should land first.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because we don't test our lints

This change is Reviewable
@highfive
Copy link

highfive commented Jul 19, 2016

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/tidy/servo_tidy/licenseck.py, python/tidy/servo_tidy/tidy.py
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 19, 2016

cc @metajack @aneeshusa
python/tidy/servo_tidy/tidy.py Outdated
lines = f.read().splitlines(True)
for idx, line in enumerate(lines):
for license in licenses_dep_toml:
ok_licensed |= (license in line)

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut Jul 19, 2016

Member

Not sure whether license would be a valid name. It's a built-in function :)

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Jul 19, 2016

Author Member

Renamed to license_line.
python/tidy/servo_tidy/tidy.py Outdated
filename = os.path.join(root, filename)
with open(filename, "r") as f:
ok_licensed = False
lines = f.read().splitlines(True)

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut Jul 19, 2016

Member

could be f.readlines()

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Jul 19, 2016

Author Member

Done.
python/tidy/servo_tidy/tidy.py Outdated
@@ -642,6 +642,23 @@ def check_wpt_lint_errors(files):
yield ("WPT Lint Tool", "", "lint error(s) in Web Platform Tests: exit status {0}".format(returncode))


def check_dep_license_errors(progress):
for root, directories, filenames in os.walk(".cargo"):

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut Jul 19, 2016

Member

It'd be better if we print something like 'Checking licenses...' before beginning the iterator.

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Jul 19, 2016

Author Member

Done.
python/tidy/servo_tidy/tidy.py Outdated
@@ -675,8 +692,10 @@ def scan(only_changed_files=False, progress=True):
errors = collect_errors_for_files(files_to_check, checking_functions, line_checking_functions)
# wpt lint checks
wpt_lint_errors = check_wpt_lint_errors(get_wpt_files(only_changed_files, progress))
# check dependecy licenses
dep_license_errors = check_dep_license_errors(progress)

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut Jul 19, 2016

Member

Also, doesn't this consume time? Wouldn't it be better if we check this when we run ./mach test-tidy --all (i.e.,) only_changed_files=False?

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Jul 19, 2016

Author Member

Fixed.
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 19, 2016

BTW, the progress indicator isn't printing correctly:

$ ./mach test-tidy --all
Checking files for tidiness...
  Progress: 100% (9297/9297)
Running the WPT lint...
  Progress: 100% (20057/20057)
Running the dependency licensing lint...
.cargo/git/checkouts/rust-clipboard-4a1584d52df56684/master/Cargo.toml:0: dependency should contain a valid license.
  Progress: 100% (1/1)2)6)

what is the magic I should be using?
python/tidy/servo_tidy/licenseck.py Outdated
'license = "MIT / Apache-2.0"',
'license = "MIT OR Apache-2.0"',
'license = "MIT"',
'license = "MIT"',

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 20, 2016

Member

dupe?

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Jul 20, 2016

Author Member

Fixed.
@aneeshusa
Copy link
Member

aneeshusa commented Jul 20, 2016

The progress_wrapper function needs an iterator as its argument, not a list like filenames, so that it can update the progress each time it yields an object. I'd recommend creating a helper function that yields Cargo.toml files and thus returns an iterator, which can then be consumed by progress_wrapper if progress is True. (See get_wpt_files for an example usage.)

Also, it would be good to have the lint run even if progress is False :)
@wafflespeanut
Copy link
Member

wafflespeanut commented Jul 20, 2016

But, the progress_wrapper converts the iterator into a list again. Let's make progress_wrapper take a list as input (and modify the related methods) instead?
python/tidy/servo_tidy/tidy.py Outdated
print '\nRunning the dependency licensing lint...'
for root, directories, filenames in os.walk(".cargo"):
if progress:
filenames = progress_wrapper(filenames)

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut Jul 20, 2016

Member

Yep, I missed this. As Aneesh said, this should be

filenames = progress_wrapper(filenames) if progress else filenames

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Jul 20, 2016

Author Member

Fixed. (Also fixed the indentation, bah humbug emacs python mode.)
@aneeshusa
Copy link
Member

aneeshusa commented Jul 20, 2016

@wafflespeanut I'd prefer to use iterators everywhere (for reduced memory usage), but your call. At the very least, I think we should still accept regular iterators as input to progress_wrapper.
@nox
Copy link
Member

nox commented Jul 20, 2016

How come do we have to do something similar to grep when we could read the files as TOML files from Python?
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 20, 2016

Yay, the progress meter now works!
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 20, 2016

@nox: it would be a bit cleaner to parse the toml file, but would probably use a bit more memory unless there's a clever streaming toml parser out there.
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 21, 2016

#12507 has landed, so we should be able to land this one. @larsbergstrom?
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 21, 2016

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

Comments from Reviewable
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 21, 2016

looks good! squash and r=me
@asajeffrey
Added test-tidy for dependent licenses.
Loading status checks…
536314b
@asajeffrey asajeffrey force-pushed the asajeffrey:test-tidy-dep-licenses branch from e003c73 to 536314b Jul 21, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 21, 2016

Squashed. @bors-servo r=larsbergstrom
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2016

📌 Commit 536314b has been approved by larsbergstrom
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2016

⌛ Testing commit 536314b with merge de4eaa4...
bors-servo added a commit that referenced this pull request Jul 21, 2016
@bors-servo
Auto merge of #12511 - asajeffrey:test-tidy-dep-licenses, r=larsbergs…
Loading status checks…
de4eaa4 
…trom

Added test-tidy for dependent licenses.

<!-- Please describe your changes on the following line: -->

Add a test-tidy lint for dependency licenses. Note that #12507 should land first.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes do not require tests because we don't test our lints

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12511)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev
@bors-servo bors-servo merged commit 536314b into servo:master Jul 21, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@larsbergstrom larsbergstrom

Labels
A-testing
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@asajeffrey @highfive @aneeshusa @wafflespeanut @nox @larsbergstrom @bors-servo
You can’t perform that action at this time.