Config: Change the way options are added

Information
Submitter:	Nilay Vaish
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 8921:9328fdaf0992
---------------------------
Config: Change the way options are added
I am not too happy with the way options are added in files se.py and fs.py
currently. This patch moves all the options to the file Options.py, functions
from which are called when required.

Testing Done

Issue Summary

Description	From	Last Updated	Status
Anyone know why we did this as opposed to import Options before? Is it something to do with relative path ...	Ali Saidi	March 25, 2012, 3:10 a.m.	Open
This bit of code to set m5_root seems like overkill since m5_root is only used to set a default path ...	Steve Reinhardt	March 28, 2012, 2:42 a.m.	Resolved

configs/example/se.py (Diff revision 1)

Anyone know why we did this as opposed to import Options before? Is it something to do with relative path import in python 2.4 vs 2.5?

Show all issues

Steve Reinhardt March 25, 2012, 11:20 a.m. (March 25, 2012, 11:20 a.m.)

No clue... I went back to where the file was created (http://repo.gem5.org/gem5/rev/49e674f2fb5d), and the Simulation.py file was created at the same time but it was imported rather than exec'd.  So it's probably just an accident of history.

configs/common/Options.py (Diff revision 1)

This bit of code to set m5_root seems like overkill since m5_root is only used to set a default path for cmd that I expect is never used.  I'd be in favor of not having a default for cmd and then getting rid of these lines.

Show all issues

Nilay Vaish March 26, 2012, 1:55 a.m. (March 26, 2012, 1:55 a.m.)

Since we don't have any check in place in se.py on the value of cmd, it is
necessary that cmd has a default value.

Steve Reinhardt March 26, 2012, 2:32 a.m. (March 26, 2012, 2:32 a.m.)

We could certainly add such a check.  It's not a big deal either way, I guess.

Nilay Vaish March 27, 2012, 9:39 a.m. (March 27, 2012, 9:39 a.m.)
```
I have added a check to se.py.
```

Description:

~		Changeset 8919:6042ba0e2f60
	~	Changeset 8921:9328fdaf0992

		Config: Change the way options are added
		I am not too happy with the way options are added in files se.py and fs.py
		currently. This patch moves all the options to the file Options.py, functions
		from which are called when required.

Diff:

Revision 2 (+158 -187)

Show changes

	configs/common/Options.py
	configs/example/fs.py
	configs/example/ruby_fs.py
	configs/example/se.py
	tests/configs/memtest-ruby.py
	tests/configs/rubytest-ruby.py
	tests/configs/simple-timing-mp-ruby.py
	tests/configs/simple-timing-ruby.py

Thanks, Nilay!

Status: Closed (submitted)

While I can understand that this may be a slightly easier way to deal with options, I'm not sure if the benefits out-weight the cost.  This is definitely one of those changes that will cause a lot of tedious downstream work.  Can we just keep it the way it is?

Gabe Black March 28, 2012, 9:11 a.m. (March 28, 2012, 9:11 a.m.)

What are the costs and the intended benefits? I'm assuming this is to reduce the amount of duplicate code which is good, but it also has the side effect of making the scripts more dependent on each other. I'm not necessarily advocating doing one thing or the other (and this is already checked in, I think), but it would be nice to be more explicit about what we're getting and what we're paying for it so we can make a more objective decision. This is the first I've looked at this, but I'm assuming Steve and Ali have already weighed the pros and cons which is why they were ok with it going in.

Generally speaking, downstream should not have a big pile of code that needs to be updated, and if it does that shouldn't prevent fixing things in the main repository which lots of other people use. The important thing to figure out is if we're improving something or just changing it.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 8919:6042ba0e2f60

Changeset 8921:9328fdaf0992

Diff:

Status: Closed (submitted)