[LNT] r307222 - Load TestSuite descriptions from json files

[Chris Matthews via llvm-commits]

reply inlined:

> On Jul 7, 2017, at 12:51 AM, Kristof Beyls <kristof.beyls at arm.com> wrote:
> 
>> 
>> On 6 Jul 2017, at 19:00, Matthias Braun <matze at braunis.de <mailto:matze at braunis.de>> wrote:
>> 
>> 
>> 
>>> On Jul 6, 2017, at 12:44 AM, Kristof Beyls <kristof.beyls at arm.com <mailto:kristof.beyls at arm.com>> wrote:
>>> 
>>> 
>>>> On 6 Jul 2017, at 01:34, Matthias Braun via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>> 
>>>> Modified: lnt/trunk/requirements.client.txt
>>>> URL: http://llvm.org/viewvc/llvm-project/lnt/trunk/requirements.client.txt?rev=307222&r1=307221&r2=307222&view=diff <http://llvm.org/viewvc/llvm-project/lnt/trunk/requirements.client.txt?rev=307222&r1=307221&r2=307222&view=diff>
>>>> ==============================================================================
>>>> --- lnt/trunk/requirements.client.txt (original)
>>>> +++ lnt/trunk/requirements.client.txt Wed Jul  5 16:34:37 2017
>>>> @@ -17,3 +17,4 @@ WTForms==2.0.2
>>>> Flask-WTF==0.12
>>>> typing
>>>> click==6.7
>>>> +pyyaml==3.12
>>> 
>>> I find it odd that this commit adds server functionality, but the pyyaml dependency is added to requirements.client.txt, not requirements.server.txt.
>>> Should this have been added to requirements.server.txt instead?
>>> Assuming this really is a bug, it's a shame the regression tests can't easily catch this kind of bug.
>> 
>> This was intentional as it isn't to clear (to me at least) where lnt server ends and lnt client starts:
>> 
>> - `lnt runtest` and `lnt submit` are obviously client and indeed don't need pyyaml
>> - `lnt runserver` is obviously server
>> - However there are lots of things like `lnt import`, `lnt checkformat`, `lnt send-daily-report`, `lnt view-comparison` that work with a database instance and hence need to be aware of the schemas. It seems they are considered "client" today or we wouldn't have all those other "server things" like "flask", "sqlalchemy" or "workzeug" in requirements.client.txt. So I figured pyyaml needs to go to requireements.client.txt as well.
> 
> IIRC, we split up the requirements.txt into requirements.client.txt and requirements.server.txt, to not require all dependencies needed for the server to be installed on bots using LNT to run tests (lnt runtest) and submit results to the server (lnt submit). So, I don't know of a "client" use case that would need these other "lnt import, lnt checkformat, ...." commands.
> It's probably not critically urgent to go and remove further "server things" like "flask, sqalchemy or workzeug" from the client requirements file, but it'd be good to know if there is a known use case for running those "lnt import, checkformat, etc" on a client, i.e. a machine that just aims to run tests and submit results to a server. Maybe Chris has a better insight on this?

I’m sure we have dependencies on Flask and SQLAlchemy in the local submission code. If there was a need to fix that, I’m sure we could. The client side dependencies are a big PITA. I would love to add a dependency to scipy and numpy, but I know those are much harder to install on most platforms.  I feel like that would be a reasonable ask for server though.

> 
>> If you wonder why yaml: I actually started my work with a json format, which works just as fine. However yaml files tend to look nicer and have 1 killer feature compared to json: They allow to put comments into the file, which I found important enough to warrant a new dependency (yaml also seems to be LLVMs go-to choice for file formats, and tools like the llvm opt-viewer depend on pyyaml as well nowadays).
> 
> I absolutely agree on the need to be able to have comments in configuration files!

I’d like to switch to yaml in more places if we can.  I’d love to be able to put comment in submission files.

> 
> Thanks!
> 
> Kristof

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170707/57a7e2fc/attachment.html>