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

Add pslint.py linter and use it on Travis CI #130

Merged
merged 1 commit into from
Mar 2, 2016

Conversation

rockdaboot
Copy link
Contributor

This adds a python PSL linter for use with Travis CI.

The former 'linting' of the PSL done via libpsl pulls in a full C project on each commit to publicsuffix/list. Configure, compile and link steps are a waste of CPU time in comparison with this simple python script.

Simone (@weppos) opened a wiki page with a bunch of tests to perform.
The python script performs all test except 'Order' (the last one in the list).

- echo "EXTRA_DIST =" >gtk-doc.make
- echo "CLEANFILES =" >>gtk-doc.make
- autoreconf --install --force --symlink
- OPTIONS="--with-psl-file=$DIR/public_suffix_list.dat --with-psl-testfile=$DIR/tests/test_psl.txt"
Copy link
Member

Choose a reason for hiding this comment

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

I see you are still referring to the test_psl.txt file. I assume this line will have to be changed once the following change is completed:

Libpsl is prepared to take the new tests.txt format in it's test suite. I will prepare a new release as soon as you pull the PR into master.

Am I correct?

@weppos weppos self-assigned this Feb 20, 2016
@weppos
Copy link
Member

weppos commented Feb 20, 2016

@rockdaboot I suggest you push an updated version of libpsl in a branch (the one with the support to run the tests from the new file format). We update this PR to pull that branch and we run all the tests.

In this way, we can test this PR from start to end before merging it into master.

@rockdaboot
Copy link
Contributor Author

@weppos I made a new libpsl branch 'newfmt' (using tests.txt as default test file) and amended .travis.yml in this PR to explicitly use tests.txt with the new format.
Hopefully, that should be it.

@weppos
Copy link
Member

weppos commented Feb 20, 2016

@rockdaboot that works for me. I'll wait for @gerv approval, as I see there are some files where the copyright is assigned to you, and I want to make sure this is fine for him in terms of acceptance.

PS. I see make configure and make clean are quite verbose. Any chance to have something less chatty? I think I may play a little bit around with output redirection.

Also, what about the libidn2 and libidn tests? Since I'm not familiar with any of these specific build configs, what is your suggestions? Should they be removed?

@weppos
Copy link
Member

weppos commented Feb 20, 2016

@gerv FYI this is a replacement for #123

@rockdaboot
Copy link
Contributor Author

@weppos Thanks for reviewing.

I see make configure and make clean are quite verbose. Any chance to have something less chatty?

./configure has a -q option to silence the configure run. CON: if something breaks, you possibly can't see why.
Also 'make' has a --silent option that stops printing all the details.
If you agree, I'll update the PR.

Also, what about the libidn2 and libidn tests?

Only, if we assume to get PSL entries that are different on IDNA2003, IDNA2008 or IDNA2008#46, e.g. the german label 'straße' (ask, If you are interested in the details). In this case we should define what PSL contains. I personally tend to test/support the latest IDNA(2008#46) since it cleans up some incompatibilities between 2003 and 2008. But I also know that most software out there relies on IDNA2003 implementations.
But anyway, we might only need one of those tests. On the other side - having all three tests allows us to be the first ones to detect/fall over incompatible PSL entries. That might be better than a 'customer' finding the problem first. The negative side is the additional waste of CPU cycles on Travis.
Long text, but I can't decide how to proceed, just try to share my knowledge here.

@weppos
Copy link
Member

weppos commented Feb 25, 2016

@gerv friendly ping for a 👍 on the copyright stuff

@weppos
Copy link
Member

weppos commented Mar 1, 2016

@rockdaboot I'm curious. Given the context of this PR, where would you see the test required in #160 (comment) implemented?

One of the reason why I am torn about the current test suite depending on libpsl (and therefore also the code here that keeps the same approach) is that we really have no test suite built into this repo. We rely on an external test suite, that doesn't necessarily have to evolve in parallel with the core PSL needs.

I'm still torn about using the libpsl in that way. I'd be curious to know where you'd see the test discussed in #160 to be implemented.

@gerv
Copy link
Contributor

gerv commented Mar 1, 2016

Copyright assigned to someone else is no problems as long as the license is open source, which it is (MIT-style). So go right ahead from a licensing perspective.

@weppos
Copy link
Member

weppos commented Mar 1, 2016

Thanks @gerv, I'll merge this PR tomorrow as it represents an enhancement with what we have so far.

@rockdaboot
Copy link
Contributor Author

Given the context of this PR, where would you see the test required in #160 (comment) implemented?

You are talking about PRIVATE entries must end in a public suffix from the ICANN section.
This is IMO not a syntax test - it needs a full featured PSL implementation. Thus, such a test does not belong into the linter.

I could add it to the libpsl test suite - it's just a few lines of code (in theory).
But how do you define 'ending' ? Let's say there is a.b.c.d in the PRIVATE section. Is the test successful if we find 'd' as ICANN public suffix ? Since we have the implicit '*' rule, there must be some logic that isn't obvious to me right now.

@weppos weppos added the r=weppos Marked as approved and ready to merge by @weppos label Mar 2, 2016
weppos added a commit that referenced this pull request Mar 2, 2016
Add pslint.py linter and use it on Travis CI
@weppos weppos merged commit e2f2f4b into publicsuffix:master Mar 2, 2016
@rockdaboot
Copy link
Contributor Author

@weppos Regarding your concerns about using libpsl...
libpsl is closely tied to this publicsuffix/list project. So any change in syntax or semantics here should be reflected by libpsl ASAP. If changes are coming or anything fails, please contact me directly (e.g. via https://github.com/rockdaboot/libpsl) and I care about it.

@weppos
Copy link
Member

weppos commented Mar 2, 2016

@rockdaboot Indeed, I appreciate your support. However, libpsl is an implementation of the list. There are PSL-specific needs or tools I don't think may be a good fit outside the scope of maintaining the PSL itself.

I think the specific validation mentioned in #160 could be one of them. I'll start adding a simple test here.

I have one more question for you. Let's assume I make some changes to the PSL locally and I want to run the tests before committing. Do you have any suggestion? Should we package the test run process in a Make task so that we can easily run it with a single one-line command, even locally?

@rockdaboot
Copy link
Contributor Author

@weppos I just made new libpsl release, so we won't need the extra branch 'newfmt' any more.

I think the specific validation mentioned in #160 could be one of them. I'll start adding a simple test here.

But how do you define 'ending' ? #160 is unclear to me (see comment above).

Should we package the test run process in a Make task so that we can easily run it with a single one-line command, even locally?

Better a shell script, e.g. 'run_tests.sh' in the main dir. We should call it from .travis.yml and it would be usable locally. I'll see if I find some time in the afternoon.

@weppos weppos mentioned this pull request Mar 5, 2016
weppos added a commit that referenced this pull request Mar 6, 2016
The folder is now created as part of the test process
since GH-130.
@weppos weppos mentioned this pull request Mar 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r=weppos Marked as approved and ready to merge by @weppos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants