validate before execution #26

[l-johanson]

This pull request includes:

• Using the validate, bind, execute pattern for runtime config files
• A refactoring using Tcl namespaces, so the code and it's scopes are easier to understand for newcomers to the project
• Validation including checking for valid_config_sessions for the linux platform

Ticket reference: #26

[nfeske]

Thank you for coming back to this issue. It is great that you were able to extend the tool after only two days of familiarizing with the language.

From looking at the patch, you definitely changed run/linux.tcl for the better! I like your introduction of the namespace along with the local functions. I agree that the scoping contributes to the readability of the code.

Let me share the following questions/remarks:

• Since the run/linux.tcl code belongs to the "run" stage of Goa, not the "build" stage, I think that calling the namespace Run would be more appropriate and intuitive. The magic spells for the run stage are located in the run/ directory whereas the building stage (support for various build systems like CMake, etc.) is covered in the build/ directory. Would you agree?

• I'd appreciate keeping the whitespace formatting of existing code untouched and let new code follow the existing patterns. E.g., I'm speaking of the change of query_attr. As a matter of style, Goa make liberal use of vertical whitespace to group things together. In particular, comments are often preceded by an empty line to make the comment appear like the title of the code that follows. The existing code is quite consistent about that. From my perspective, different tastes notwithstanding, it would be nice to uphold consistency.

• Could you clarify your intention behind query_attrs? I'm admittedly a bit confused because attributes of XML nodes are supposed to be unique. Maybe the addition of an example use as a comment would help to make the distinction from query_attr clear?

• I have not yet looked at the changes of linux.tcl side by side. The diff is (perfectly understandably) not quite digestible. So I need some more time to compare both versions. From first glance, I like what I see. ;-)

[nfeske]

After sleeping a night over it, two more thoughts were crossing my mind.

• I think that the formalization "validate, bind, execute" you introduced is good. But the sequence of operations that you added to bin/goa should better be moved into a function local to lib/run/linux.tcl so that the interface between bin/goa and the run stage (like run/linux.tcl) becomes as small as possible, and giving the run stage the maximum freedom for implementation. I think down the road, other run-stage variants will likely adopt the formalism as a good practice, but not by force but by choice.

• As you recognized in Please provide a way to run a scenario based on qemu #32, Goa's structure foresees the potential for run stages other than linux. To foster modular design, bin/goa should better not explicitly call Run::Linux::executeor Run::Qemu::execute but one agreed-upon top-level function that is provided by each run-stage implementation, let's say Run::execute, which would internally call functions within the Run::Linux:: namespace. The selection of the run stage to use is only a matter of including the right lib/run/*.tcl file, which would be selectable by a command-line argument like --target linux or --target qemu.

[jschlatow]

@l-johanson Thanks for creating this PR and your other suggestions about improving Goa. I like where this is going. In fact, we recently moved the repository from nfeske/goa to genodelabs/goa because we are planning to invest more efforts into this as a team in order to make Goa more useful. Your suggestions perfectly fall in line.

Hence, my question is: Are you planning to invest more time into this PR in order to address Norman's feedback? Do you need any further assistance with that?

[jschlatow]

I'm closing this PR because most of it has been addressed in the scope of #44.