search

GaloisInc / csaf

Control Systems Analysis Framework - a framework to minimize the effort required to evaluate, implement, and verify controller design (classical and learning enabled) with respect to the system dynamics.
BSD 3-Clause "New" or "Revised" License
11 stars 4 forks source link

practices like validate_dir are overly restrictive and should not be used #73

Closed podhrmic closed 3 years ago

podhrmic commented 3 years ago

In GitLab by @zutshi on Oct 1, 2020, 18:44

Validating the name of the directory is not a good practice. It is overly restrictive, and can not even achieve its purpose.

https://gitlab-ext.galois.com/assuredautonomy/csaf_architecture/-/blob/develop/run-csaf.sh#L6

podhrmic commented 3 years ago

In GitLab by @podhrmic on Oct 2, 2020, 09:15

Can you please elaborate? Without the directory validation, if you run the script from some other directory your paths will break.

podhrmic commented 3 years ago

In GitLab by @zutshi on Oct 19, 2020, 19:03

I understand the reason, but I will suggest to make the scripts more resilient and let them fail when they have to. And, augment it with a warning which indicates the possibility of running the scripts from the wrong place.

In general, erroring out due to the name of the current directory is a brittle practice which might lead to wrong assumptions down the road. It is not useful and breaks useful and often desired things like renaming csaf_architecture, dynamic paths, etc. Ideally, one should just throw the error on the specific path that breaks, instead of a blanket requirement.

podhrmic commented 3 years ago

In GitLab by @podhrmic on Oct 20, 2020, 17:36

Agreed that we can do better here. Part of the solution would be resolving #89, another part would be to rewrite run-csaf.sh in python (the shell script is getting pretty complex, and can be easily broken). I think this is a "nice to have" feature - the current script tells you if you are running from the wrong directory, and we can explicitly mention this in the README. If we have extra time, then we can address it properly.

podhrmic commented 3 years ago

In GitLab by @zutshi on Oct 20, 2020, 19:27

Well, I think the solution that I mentioned was pretty simple: replace the exit with a warning instead.

podhrmic commented 3 years ago

In GitLab by @podhrmic on Oct 28, 2020, 08:33

mentioned in commit b61202e36dafdb6f288f587c3299da0fe658ab30