Make pylint and jsl optional #502

tiran · 2017-02-24T07:42:47Z

./configure no longer fails when pylint or jsl are not available. The
make targets for pylint and jsl are no longer defined without the tools.

Rational:
pylint and jsl are not required to build FreeIPA. Both are useful
developer tools. It's more user friendly to make both components
optionally with default config arguments. There is no reason to
fail building on a build system without development tools.

It's still possible to enforce dependency checks with --with-jslint and
--enable-pylint.

https://fedorahosted.org/freeipa/ticket/6604

Signed-off-by: Christian Heimes cheimes@redhat.com

tkrizek · 2017-03-01T09:22:58Z

Please clean up the confgure.ac and update freeipa.spec file as well.

configure.ac:375:       AS_HELP_STRING([--disable-pylint],
freeipa.spec.in:16:    %global enable_pylint_option --disable-pylint
freeipa.spec.in:17:    %global without_jslint_option --without-jslint

tiran · 2017-03-01T09:32:58Z

good catch, @tomaskrizek

lslebodn · 2017-03-01T09:51:20Z

May I know why the default was changed without design discussion?

IIRC pscacek intentionally enabled it by default. Much better approach would be to print hint at configure time detection that it is optional and can be disabled.

Code wise; the change is too complicated (too many nested if condition ...)
which does not improve readability of configure.ac

lslebodn

Code wise; the change is too complicated (too many nested if condition ...)
which does not improve readability of configure.ac

tiran · 2017-03-01T10:02:02Z

Please see ticket for reasoning.

My solution is the best thing, I could come up with in short time. It's not worth the trouble to burn a lot of time on it. It's write-once code. You are welcome to provide a better solution. I know your autoconf Fu is stronger than mine.

lslebodn · 2017-03-01T10:07:53Z

Writing hint together with error is the simplest solution. And still remind developers to install pylint/jslint.

e.g.

diff --git a/configure.ac b/configure.ac
index af41f5e3a..c02dd1fe4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -384,7 +384,10 @@ if test x$PYLINT != xno; then
     AC_MSG_CHECKING([for Pylint])
     $PYTHON -m pylint --version > /dev/null
     if test "$?" != "0"; then
-        AC_MSG_ERROR([cannot find pylint for $PYTHON])
+        AC_MSG_ERROR([cannot find pylint for $PYTHON
+This feature is optional and aimed for developers. You can skip this check
+wich configure time option --disable-pylint
+        ])
     else
         AC_MSG_RESULT([yes])
     fi

lslebodn · 2017-03-01T10:55:26Z

Please add explanation to the thumb down

lslebodn · 2017-03-01T13:08:25Z

It was explained on IRC

< cheimes> lslebodn: Your proposal is missing the point of the ticket. It doesn't not simplify
building, but rather improves error messages. The whole point of the ticket is a more
pleasant experience for outsiders that are not FreeIPA core contributors.

But there is main problem with this PR. The design document expected these option enabled by default.
http://www.freeipa.org/page/V4/Build_system_refactoring

This is a reason why I mentioned to log just a hint for optional *lint dependencies.

And ticket https://pagure.io/freeipa/issue/6604 says that options were made optional a moth ago

master:

5c18fea CONFIGURE: Fix detection of pylint
3f91469 CONFIGURE: Update help message for jslint
b82d285 SPEC: Fix build in mock

tkrizek · 2017-03-01T13:15:33Z

@lslebodn We don't want to have linters enabled by default when you run ./configure without options. But you're right we should update the wiki pages to mention the new defaults.

lslebodn · 2017-03-01T13:19:34Z

But you're right we should update the wiki pages to mention the new defaults.
Such change require broader discussion.

e.g. I know that @rcritten had strong opinion about pylint usage in past.

tiran · 2017-03-01T13:23:57Z

@tomaskrizek good point, I added a TODO item to the ticket, https://pagure.io/freeipa/issue/6604#comment-415669

tkrizek · 2017-03-01T13:39:41Z

Wiki page updated (along with `--without-ipatests`` option from #364).

@lslebodn Ok, let's keep the PR open for a couple days to see if there's any disagreement. I don't see this as a drastic change that should be widely discussed, but feel free to bring up the topic on freeipa-devel if you disagree.

MartinBasti · 2017-03-01T13:48:26Z

Since we have gating here each PR is checked by linters, commits are checked before pushed, that was reason why linters are optional now in build.

pvoborni · 2017-03-01T17:39:50Z

+1 Reasoning for not skipping linters was that reviewer or patch author can forget to run those. This problem was solved by travis checks.

lslebodn · 2017-03-01T20:18:13Z

On (01/03/17 09:39), Petr Vobornik wrote: +1 Reasoning for not skipping linters was that reviewer or patch author can forget to run those. This problem was solved by travis checks.

ATM nothing force reviewer/author to run lint. `makerpms.sh` does not call `make lint` and it is not a dependency of `make all`. configure script just remind developer to install pylint/jslint. (or disable configure time check) Which is a huge difference. If you think that developers should not/needn't have installed pylint by default then it's your decission. But I cannot see a good reason for removing this reminder. my 2 cents

HonzaCholasta · 2017-03-02T06:37:41Z

I tend to agree with @lslebodn, but I don't have a strong opinion on this. I noticed a couple of issues though:

--without-jslint does not seem to work correctly:

$ ./configure --without-jslint
...
                    IPA Server 4.4.90.dev201703020634+git3a29b47
                    ========================
...
        jslint:                   /usr/bin/jsl
...

In freeipa.spec.in, when with_lint is not defined, lint should be disabled, so --disable-pylint and --without-jslint should be passed to %configure.

lslebodn · 2017-03-02T07:57:05Z

On (01/03/17 22:37), Jan Cholasta wrote: I tend to agree with @lslebodn, but I don't have a strong opinion on this. I noticed a couple of issues though: * `--without-jslint` does not seem to work correctly: ``` $ ./configure --without-jslint ... IPA Server 4.4.90.dev201703020634+git3a29b47 ======================== ... jslint: /usr/bin/jsl ... ``` * In `freeipa.spec.in`, when `with_lint` is not defined, lint should be disabled, so `--disable-pylint` and `--without-jslint` should be passed to `%configure`.

This is exactly a reason why the simplest solution would be to exend error messages without changing default. Default "yes" should encourage downstream packagers[1,2] to run make lint and catch issues with backported(downstream only) patches. BTW there were also some version of packages for opensuse[2] but they were removed. [1] https://anonscm.debian.org/cgit/pkg-freeipa/freeipa.git [2] https://en.opensuse.org/Portal:FreeIPA

stlaz · 2017-03-02T08:24:56Z

There's an ongoing discussion about the acceptance of the patch. Removing the ACK label until the acceptance is agreed on. Please, @lslebodn or @tomaskrizek, add the label back once that is done. However, please, try to cut the discussion short and make the decision in the least comments possible.

tkrizek · 2017-03-02T09:40:26Z

Since --without-jslint was removed, there's actually no way to explicitly turn off jsl (it will always be autodetected). I tried to set --with-jslint=no, but that didn't do the trick.

Pylint can be disabled with --enable-pylint=no, however.

I suggest the following:

when --with-jslint=no, turn off jsl,
pass --with-jslint=no --enable-pylint=no to %configure in freeipa.spec.in when with_lint is not defined.

tiran · 2017-03-02T11:04:03Z

@tomaskrizek autoconf is a bit magic. --without-jslint is still there. The line AC_ARG_WITH([jslint], ...) provides --with-jslint and --without-jslint. But there was a bug in my check logic.

I pushed another fix that fixed a bug in my logic and replaces some complicated checks with a straight-forward AS_CASE block.

tkrizek · 2017-03-02T11:25:40Z

freeipa.spec.in

@@ -11,10 +11,7 @@
 # lint is not executed during rpmbuild
 # %%global with_lint 1
 %if 0%{?with_lint}
-    %global enable_pylint_option --enable-pylint
-%else


As @HonzaCholasta mentioned, if with_lint is not defined, linting should be disabled.

tiran · 2017-03-02T11:27:22Z

Which audience is our primary concern here?

Should default settings be tailored towards downstream packager?
Or should defaults settings be user-friendly for upstream and external users?

I'm for upstream first.

Packaging is pretty much automated and scripted. A packager can easily adjust a script for a new version. There also just a handful of distributions (Fedora/RHEL/CentOS, Debian/Ubuntu, SuSE, Gentoo).

tkrizek · 2017-03-03T11:31:11Z

@tiran Needs rebase.

lslebodn · 2017-03-03T12:12:56Z

I am still expect some comment from @rcritten LS

tiran · 2017-03-14T16:56:23Z

The PR got three +1 / heart and not -1. I propose to get it merged for 4.5 today.

./configure no longer fails when pylint or jsl are not available. The make targets for pylint and jsl are no longer defined without the tools. Rational: pylint and jsl are not required to build FreeIPA. Both are useful developer tools. It's more user friendly to make both components optionally with default config arguments. There is no reason to fail building on a build system without development tools. It's still possible to enforce dependency checks with --with-jslint and --enable-pylint. https://fedorahosted.org/freeipa/ticket/6604 Signed-off-by: Christian Heimes <cheimes@redhat.com>

lslebodn · 2017-03-14T19:17:10Z

On (14/03/17 09:56), Christian Heimes wrote: The PR got three +1 / heart and not -1. I propose to get it merged for 4.5 today.

I cannot see 3-times +1 in this PR. I can see just 2 reviewers in top right corner * lslebodn requested changes * tomaskrizek approved these changes Is there any reason why upstream discussion is not accepted? https://www.redhat.com/archives/freeipa-devel/2017-March/msg00371.html LS

tiran · 2017-03-14T23:04:59Z

#502 (comment)

two thumbs up, one heart, no thumbs down

HonzaCholasta · 2017-03-15T06:11:58Z

This PR makes packaging IPA 4.5 on RHEL 7 easier for me, so thumbs up from me.

tiran · 2017-03-15T07:52:29Z

PR #593 addresses @lslebodn concerns and provides a superior solution for local pre-commit checking.

lslebodn · 2017-03-15T11:44:52Z

PR #593 is not related to default yes; It is about something else.

Current version does not fix concerns; because default should be yes as it was discussed in https://www.redhat.com/archives/freeipa-devel/2017-March/msg00371.html

I looks like upstream discussion is useless. And nobody cares about other distributions then fedora/rhel which can parse recommendation form upstream spec file.

I am really disappointed from such upstream unfriendly approach.

lslebodn · 2017-03-15T11:52:54Z

This PR makes packaging IPA 4.5 on RHEL 7 easier for me, so thumbs up from me.

I understand it is more convenient to have less extra configure options in rhel;
But it was discussed on upstream mailing list and better error messages would give such hints to everyone. https://www.redhat.com/archives/freeipa-devel/2017-March/msg00308.html

@HonzaCholasta it would be good if you add comment also to upstream discussion; if you prefer autodetection. It would be good if result of discussion is the same as pushed patch.
https://www.redhat.com/archives/freeipa-devel/2017-March/msg00371.html

Current version will not persuade other distributions(debian; openSUSE) to run pylint as part of build.

lslebodn · 2017-03-15T12:02:19Z

On (14/03/17 16:05), Christian Heimes wrote: #502 (comment) two thumbs up, one heart, no thumbs down

My naive assumption was that discussion about apprach was moved to freipa-devel.

HonzaCholasta · 2017-03-15T12:17:04Z

@lslebodn, nobody said that this has to be the last lint build related patch ever, we can change the behavior later, even on top of this PR. I would rather push this now and continue the discussion / submit additional PRs after 4.5 is released.

lslebodn · 2017-03-15T12:25:49Z

On (15/03/17 05:17), Jan Cholasta wrote: @lslebodn, nobody said that this has to be the last lint build related patch ever, we can change the behavior later, even on top of this PR. I would rather push this now and continue the discussion / submit additional PRs after 4.5 is released.

But it would be good to have patch/approach in **official release** which was result of upstream discussion. Does it mean that I should improve error messages myself? LS

HonzaCholasta · 2017-03-15T12:31:26Z

4.5.1 will be an official release too.

pvoborni · 2017-03-15T12:32:50Z

There was no result in the upstream discussion. My personal opinion is that one way or the other can work. They are for different use cases. I tend to prefer the "be easier for developer" approach. That said, preferred method for downstreams needs to be documented ideally in BUILD.txt.

In any case spending so much time discussing so minor change is a waste of time. I'd push it.

tiran · 2017-03-15T12:35:27Z

@pvoborni For the use case "easy for developers" the make lint target is not sufficient. It tests only a small subset and doesn't check Python 3 issues. PR #593 provides a better alternative for a pre-commit patch check that takes care of linting on Python 2 and 3 plus additional checks.

lslebodn · 2017-03-15T12:37:06Z

On (15/03/17 05:32), Petr Vobornik wrote: In any case spending so much time discussing so minor change is a waste of time. I'd push it.

Will you accept patch whith improves error messages? I can send it in few minutes; I do not want to creat PR which will be rejected. LS

HonzaCholasta · 2017-03-15T12:38:03Z

master:

f1f6350 Make pylint and jsl optional

pvoborni · 2017-03-15T12:38:59Z

If it improves messages then I assume so provided that in won't be controversial in other aspects.

tkrizek self-assigned this Mar 1, 2017

tiran force-pushed the optional_linters branch from 21794f9 to 3a29b47 Compare March 1, 2017 09:42

lslebodn suggested changes Mar 1, 2017

View reviewed changes

tiran force-pushed the optional_linters branch from b5c26e5 to 3a29b47 Compare March 1, 2017 13:10

tkrizek added the ack Pull Request approved, can be merged label Mar 1, 2017

stlaz removed the ack Pull Request approved, can be merged label Mar 2, 2017

tiran force-pushed the optional_linters branch from 3a29b47 to bf8bd4e Compare March 2, 2017 11:04

tkrizek suggested changes Mar 2, 2017

View reviewed changes

tiran force-pushed the optional_linters branch from bf8bd4e to 17aaddc Compare March 2, 2017 11:30

tkrizek approved these changes Mar 2, 2017

View reviewed changes

tiran force-pushed the optional_linters branch from 17aaddc to 4a642a9 Compare March 3, 2017 11:52

tkrizek removed the ack Pull Request approved, can be merged label Mar 3, 2017

tiran force-pushed the optional_linters branch 2 times, most recently from 12e21b6 to fe062ff Compare March 14, 2017 16:54

tiran force-pushed the optional_linters branch from fe062ff to e6d392d Compare March 14, 2017 17:11

tkrizek added the ack Pull Request approved, can be merged label Mar 15, 2017

HonzaCholasta added the pushed Pull Request has already been pushed label Mar 15, 2017

HonzaCholasta closed this Mar 15, 2017

tiran deleted the optional_linters branch March 15, 2017 12:38

HonzaCholasta mentioned this pull request Mar 15, 2017

spec file: support client-only build #596

Closed

tiran mentioned this pull request Mar 15, 2017

CONFIGURE: Improve error messages for optional dependencies #599

Closed

Make pylint and jsl optional #502

Make pylint and jsl optional #502

Conversation

tiran commented Feb 24, 2017 • edited by HonzaCholasta

tkrizek commented Mar 1, 2017

tiran commented Mar 1, 2017

lslebodn commented Mar 1, 2017

lslebodn left a comment

Choose a reason for hiding this comment

tiran commented Mar 1, 2017

lslebodn commented Mar 1, 2017

lslebodn commented Mar 1, 2017

lslebodn commented Mar 1, 2017

tkrizek commented Mar 1, 2017

lslebodn commented Mar 1, 2017

tiran commented Mar 1, 2017

tkrizek commented Mar 1, 2017

MartinBasti commented Mar 1, 2017

pvoborni commented Mar 1, 2017

lslebodn commented Mar 1, 2017 via email

HonzaCholasta commented Mar 2, 2017

lslebodn commented Mar 2, 2017 via email

stlaz commented Mar 2, 2017

tkrizek commented Mar 2, 2017

tiran commented Mar 2, 2017

tkrizek Mar 2, 2017

Choose a reason for hiding this comment

tiran commented Mar 2, 2017

tkrizek commented Mar 3, 2017

lslebodn commented Mar 3, 2017 via email

tiran commented Mar 14, 2017

lslebodn commented Mar 14, 2017 via email

tiran commented Mar 14, 2017

HonzaCholasta commented Mar 15, 2017

tiran commented Mar 15, 2017

lslebodn commented Mar 15, 2017 • edited

lslebodn commented Mar 15, 2017 • edited

lslebodn commented Mar 15, 2017 via email

HonzaCholasta commented Mar 15, 2017

lslebodn commented Mar 15, 2017 via email

HonzaCholasta commented Mar 15, 2017

pvoborni commented Mar 15, 2017

tiran commented Mar 15, 2017

lslebodn commented Mar 15, 2017 via email

HonzaCholasta commented Mar 15, 2017

pvoborni commented Mar 15, 2017

tiran commented Feb 24, 2017 •

edited by HonzaCholasta

lslebodn commented Mar 15, 2017 •

edited

lslebodn commented Mar 15, 2017 •

edited