Fixed support for the $PROJECT variable in create_test (as well as other minor fixes in the scripts directory)

[worleyph]

• Fix support for the $PROJECT variable in the create_test script.

Environmental variables in $CESMSCRATCHROOT are expanded immediately in create_list, before $PROJECT can be set. The logic was changed to delay this expansion for $PROJECT. Code for setting $PROJECT was cleaned up in both create_list and create_newcase.

Note that, based on my needs and work, Bill Sacks is looking at a more comprehensive approach, one that will go into both the NCAR version and in ACES4BGC, and will be available to ACME when this is complete.

• Fix a bug related to the $PROJECT variable in machine specific files for Edison, Hopper, Mira, and Titan.

A test for whether $PROJECT has been defined looked for a null string. Subsequent changes in the design set the default to PROJECT_UNSET. The test was changed to look for this string instead.

• Eliminate the error message about this not being an SVN repository when the GIT/SVN provenance is being saved as part of the model run.

This is a placeholder fix to look for .git and .svn before executing git describe and svn info . This eliminates the error message (about this not being an SVN repository) for the ACME branch until something better is checked in.

[BFB]

[rljacob]

Can you add a bit more detail on the commit message? What support was fixed and what bug was fixed?

[worleyph]

Okay ... the detail is in the code diff itself as well (and is pretty
simple) but I will expand on this.

Pat

On 8/22/14 11:16 AM, Robert Jacob wrote:

Can you add a bit more detail on the commit message? What support was
fixed and what bug was fixed?

—
Reply to this email directly or view it on GitHub
#6 (comment).

[douglasjacobsen]

In general, I would say the commit message it self should give someone enough information to understand the purpose of the commit. If someone wants to see how something particular was implemented, they can always see the diff in more detail, but diffs aren't that easy to understand so we shouldn't expect everyone (or anyone) would actually look at the diffs.

With the exception of pull requests. where the reviewer is expected to at least look over the full diff (though not each individual commit's diff).

[worleyph]

Understood - in the future I will provide more complete explanations in
the commit tags and (perhaps?) only summarize this in the pull request.
I'm still trying to map my previous experience with CESM check-ins,
where commits (and the corresponding ChangeLog entries) were more on par
with pull requests (being much larger granularity). It didn't have to be
this way, but the overhead of testing for check-ins made this a
practical necessity. Of course, I was primarily working directly with
the trunk and not with branches back then.

Thanks.

(Still in training) Pat

On 8/24/14 5:05 PM, Doug Jacobsen wrote:

In general, I would say the commit message it self should give someone
enough information to understand the purpose of the commit. If someone
wants to see how something particular was implemented, they can always
see the diff in more detail, but diffs aren't that easy to understand
so we shouldn't expect everyone (or anyone) would actually look at the
diffs.

With the exception of pull requests. where the reviewer is expected to
at least look over the full diff (though not each individual commit's
diff).

—
Reply to this email directly or view it on GitHub
#6 (comment).

[jedbrown]

Just a comment that the primary reason to use clean modest-sized commits is to make reviewing and possibly future debugging easier. We don't want to merge a branch that starts with a commit that introduces bugs, only to be fixed at some later point by a commit that fixes the bugs. Ideally, each commit is an atomic change that can be reviewed, tested, and declared good on its own. When refactoring, often one commit will simply rename an interface or add an argument, updating all callers for the new interface. The reviewer checks that the new name/interface is reasonable and can move on comfortable that there is no behavioral change. The behavioral change comes in a subsequent commit that has a much smaller diff consisting only of the new feature, the new call site, and a test confirming that it does what it is supposed to do. Reading the "full diff" mostly only makes sense when the original commits were poorly factored.

[worleyph]

No argument from me. The branch referred to in the subject also does not
have this "feature" - its only problem is that is has too brief commit
messages?

If desired I can start over with this branch and submit a new pull
request in which commit messages are more descriptive, to go along with
the pull request comments. I do believe however that there is some need
for the mods in this branch. Please tell me what you want me to do in
order to get this merged in.

Note that the error that this branch corrects reflects a lack of
coverage in my testing - create_test behaves differently when specifying
a single test inline and when reading in a test from a file - not
something that I would have expected, and definitely not clear from
reading the code. The behavior being addressed - expansion of some
environment variables immediately and some not until later - also was
not something that I expected. Again, Bill Sacks is looking at removing
this anomalous behavior. However, I "hurried" this current fix -
adequate though perhaps not as elegant as it could be - because it was
my understanding that the current trunk is causing issues with the
testing at NERSC and on the LCF systems.

On 8/25/14 7:00 PM, Jed Brown wrote:

Just a comment that the primary reason to use clean modest-sized
commits is to make reviewing and possibly future debugging easier. We
don't want to merge a branch that starts with a commit that introduces
bugs, only to be fixed at some later point by a commit that fixes the
bugs. Ideally, each commit is an atomic change that can be reviewed,
tested, and declared good on its own. When refactoring, often one
commit will simply rename an interface or add an argument, updating
all callers for the new interface. The reviewer checks that the new
name/interface is reasonable and can move on comfortable that there is
no behavioral change. The behavioral change comes in a subsequent
commit that has a much smaller diff consisting only of the new
feature, the new call site, and a test confirming that it does what it
is supposed to do. Reading the "full diff" mostly only makes sense
when the original commits were poorly factored.

—
Reply to this email directly or view it on GitHub
#6 (comment).

[jedbrown]

I don't have a problem with the commit sizes, but (especially being
unfamiliar with the scripts being touched), am not in a position to
evaluate whether they are correct. This would be helped by more
descriptive commit messages (a good thing), but it also helps to have
someone familiar with the component reviewing commits. One other
comment on the commit messages is that it's good to keep the subject
line reasonably short (most projects ask for 72 chars). Further
explanation comes in subsequent paragraph(s). I also like to label
components for easier high-level browsing, something like:

scripts: provenance: check for .git, .svn before git describe, svn info

Calling "git describe" or "svn info" when not in a repository prints
to stderr, nothing on stdout, and exits with nonzero. Since files are
subsequently copied around, it is easier to check first than to
redirect stderr to /dev/null and process failure of "git describe"
when we are not in a Git repository.

[worleyph]

Slight change of topic ... Bill Sacks is in the middle of refactoring
what I did in this branch. In the process, he (just now) realized that
create_clone also needs to be modified, and a number of the tests call
create_clone as part of their logic.

So, my pull request is not sufficient to solve all of the issues with
$PROJECT and the testing logic. I believe that everything works
currently if $PROJECT is set as an environment variable, and that should
be sufficient for now.

Is there a way for me (or you) to cancel the pull request? If so, I'll
do so and then wait until Bill Sacks finishes reworking this logic. At
that time I'll recreate my branch and try again.

Thanks.

Pat

On 8/25/14 7:46 PM, Jed Brown wrote:

I don't have a problem with the commit sizes, but (especially being
unfamiliar with the scripts being touched), am not in a position to
evaluate whether they are correct. This would be helped by more
descriptive commit messages (a good thing), but it also helps to have
someone familiar with the component reviewing commits. One other
comment on the commit messages is that it's good to keep the subject
line reasonably short (most projects ask for 72 chars). Further
explanation comes in subsequent paragraph(s). I also like to label
components for easier high-level browsing, something like:

scripts: provenance: check for .git, .svn before git describe, svn info

Calling "git describe" or "svn info" when not in a repository prints
to stderr, nothing on stdout, and exits with nonzero. Since files are
subsequently copied around, it is easier to check first than to
redirect stderr to /dev/null and process failure of "git describe"
when we are not in a Git repository.

—
Reply to this email directly or view it on GitHub
#6 (comment).

[jedbrown]

Closed pending updates from Bill Sacks. This is done using the "Close and comment" button at the bottom of the PR web page.