client: add <ignore_tty> config file option (Unix)

[davidpanderson]

TTY devices starting with the given string(s) will be skipped
in checking for system idleness.

Fixes #3036

[ChristianBeer]

I know that the XML in cc_config.xml is not really XML compliant. But could we make new features use the correct syntax? This would mean that multiple tags are encapsulated in a "container" tag. The config for this feature would than look like this:

<cc_config>
    <options>
        <ignore_tty_devs>
            <ignore_tty>/dev/tty0</ignore_tty>
        </ignore_tty_devs>
    </options>
</cc_config>

[davidpanderson]

I think the syntax if fine as is.

[davidpanderson]

Can someone please merge this???
This provides a feature that a user requested.

[ChristianBeer]

Besides my objection regarding the correct XML syntax the original requester already said that there are still problems with this:

The vector tty_list is a static and is only filed during start of BOINC. As such any tty device plugged in after BOINC started is not detected. That's not ok.

[davidpanderson]

That's a separate issue. BOINC has never looked for new devices after startup.

[RichardHaselgrove]

Another reason why this should be handled dynamically: laptops with docking stations.

https://boinc.berkeley.edu/forum_thread.php?id=12921&postid=91268#91268

[davidpanderson]

The client currently has a hardwired list of directories. This PR makes it configurable. This is an improvement. Periodically looking for directories is a separate issue.

[RichardHaselgrove]

USB devices (as per original problem) and docking stations are intended to be hot-swappable: KVMs might also come under that heading, too.

We ought to move into the 21st century sometime.

[vwbusguy]

This PR is still an incremental improvement that would potentially help a great number of Linux users. Right now, I need to toggle GPU on and off whenever I want it to run on idle, which, is a much more painful experience than possibly having to restart the boinc client when I re-dock my laptop.

[TheAspens]

@ChristianBeer and @RichardHaselgrove - This change improves the current function of the code and thus represents an improvement. You are both correct that a totally different approach to this code be better but that is a much larger change and should not invalidate the usefulness of merging this change. Can you guys agree to that so that this pull request can move forward?

[TheAspens]

@ChristianBeer - both of these are valid XML based on https://www.xmlvalidation.com/

<cc_config>
    <options>
        <ignore_tty_devs>
            <ignore_tty>/dev/tty0</ignore_tty>
        </ignore_tty_devs>
        <ignore_tty_devs>
            <ignore_tty>/dev/tty0</ignore_tty>
        </ignore_tty_devs>
    </options>
</cc_config>

and

<cc_config>
    <options>
           <ignore_tty>/dev/tty0</ignore_tty>
           <ignore_tty>/dev/tty0</ignore_tty>
    </options>
</cc_config>

Both cases have a repeating element. Can you explain what you feel that this is not correct syntax?

To put it a different way your solution repeats the <ignore_tty_devs> tag while David's repeats the <ignore_tty_dev> or more visually look this:

<cc_config>
    <options>
        <ignore_tty_devs><ignore_tty>/dev/tty0</ignore_tty></ignore_tty_devs>
        <ignore_tty_devs><ignore_tty>/dev/tty0</ignore_tty></ignore_tty_devs>
    </options>
</cc_config>

vs

<cc_config>
    <options>
        <ignore_tty>/dev/tty0</ignore_tty>
        <ignore_tty>/dev/tty0</ignore_tty>
    </options>
</cc_config>

Both are valid. If there were multiple related tags being used - then I would agree that @ChristianBeer solution is better. However, since it is only a single repeated tag - I see them as both valid and equivalent.

[RichardHaselgrove]

Re #3044 (comment)

Yes, I'm content to do this incrementally: fix the static/startup list now, return for dynamic device management in another PR.

[ChristianBeer]

For the record my solution would look like this:

<cc_config>
    <options>
        <ignore_tty_devs>
            <ignore_tty>/dev/tty0</ignore_tty>
            <ignore_tty>/dev/tty1</ignore_tty>
        </ignore_tty_devs>
    </options>
</cc_config>

This is not an issue of validation but rather practicability. Parsing the XML into a proper tree and traversing through a proper tree is better than a self-cooked solution. This becomes more clear when you add other options to the XML. It's always better to encapsulate elements that can occur multiple times (ignore_tty) into a single parent element (ignore_tty_devs) that can be parsed into a list or array. Otherwise you only have a list of options where you need to traverse the whole list in order to get the information you want.

Because this is a central config file I believe at least this issue of encapsulating the XML elements should be dealt with in this PR and not put of until a alter time. This adds just more technical debt so I'm still opposed to this PR as is.

[TheAspens]

@davidpanderson and @ChristianBeer - can you guys both be on the contributor call next Thursday (June 27th?) so that you two can discuss and move this forward? It would be better if you guys could meet and discuss before that so that we can get the final code into the 7.16 release.

[davidpanderson]

There is no advantage in having an enclosing tag.
The config file has other elements that can be repeated, with no enclosing tag:

<alt_platform>

<exclude_gpu>
<exclusive_app>
<exclusive_gpu_app>
<ignore_ati_dev> etc.

Other than pedantry, I see no reason to change notation at this point.
I really have nothing more to say about this.

[ChristianBeer]

I wrote all my arguments and have nothing to add in a phone call. The argument that we should go with the quick to implement fix to a solution (and add a proper implementation later) is what lead us into the current situation. Where there this a horrific amount of technical debt that is not easily dealt with because of compatibility issues and nobody willing to do the proper implementation. If we continue in this way for the sake of speed, we run into the problem that the technical debt can never be fixed and BOINC needs to be rebuild from scratch (loosing compatibility).

I'm not removing my "No" Vote and would refer to the Governance document on how to handle this: https://github.com/BOINC/boinc-policy/blob/master/Governance_Documents/Governance.md#511-consensus-voting

[ChristianBeer]

I closed this PR by accident. Sorry

[lfield]

@ChristianBeer While I don’t disagree with your desire to improve the XML, I feel that enforcing this PR to adhere to your proposed convention is unfair and creates friction for the development process. A convention to follow should first be agreed before it is enforced. If our desire is to improve the XML by a style guide or the use of a standard library, the approach should first be agreed in an issue before PRs are made to adhere to that convention. As such I don’t think that this should block the PR as it is following the defacto approach already employed in the code base.

[TheAspens]

@AenBleidd @SETIguy (and anyone else) - what are your thoughts on this disagreement about the XML format?

[TheAspens]

I should have invited any/all of the @BOINC/committer's to comment on the disagreement about the XML format. Please offer your comments on the right way to move forward on this.

[tristanolive]

Having done a lot of work on preferences XML for GridRepublic and the Drupal integration, I very much agree that the formatting approach Christian offers is the right way to go. Since there is a dispute, I also agree with Laurence that this "right way" needs to be adopted prior to being a requirement for PRs, so this particular PR should not be blocked.

If there is significant technical debt to overcome with regards to XML formatting and parsing, maybe that approach can just as easily be deprecated in favor of JSON or an otherwise more appropriate format for data exchange? It might not be worth fixing XML if, for instance, there is a use case for adding support for rapidjson instead.

[adamradocz]

I agree with @ChristianBeer. The custom XML parser is causing a lot of issues all over the project. A standard library should have been used in the first place. I think we have to draw a line and not to merge any new quick fixes, but see the long-run and switch the XML parser to a standard library (XML, JSON whatever).
The question is where to draw the line? I think this one should be accepted, as it is already done, but any future XML quick fixes are not, so we are forced to work on switching to a standard library.

[TheAspens]

Based on reviewing the comments above, I suggest the following is my summary and my recommendation based on the consensus above:

• Both versions of the XML under discussion are valid XML and can be handle by either a standard XML libraries as well as BOINC custom XML library
• The formatting that @ChristianBeer proposes is an improved format for the XML but as it represents a change to the existing defacto XML document format style, it shouldn't be imposed as a requirement for accepting a pull request
• We should define a format style for XML documents and once agreed to, future pull requests should comply with the new XML document format style

I recommend the following:

• @ChristianBeer accept this pull request in its current state
• @ChristianBeer and others work on Issue Define XML code style #3242 to define a XML document format style for the future. I would also suggest we discuss this on the next contributor call.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@4411ef5). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3044   +/-   ##
=========================================
  Coverage          ?   15.33%           
=========================================
  Files             ?       40           
  Lines             ?     6410           
  Branches          ?        0           
=========================================
  Hits              ?      983           
  Misses            ?     5427           
  Partials          ?        0

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4411ef5...7b6cda7. Read the comment docs.