SConstruct (Diff revision 1)

I know you didn't change this, but should we check for both hooks?  (I added the pre-qrefresh one a while ago)

1. Steve Reinhardt May 2, 2011, 5:58 a.m. (May 2, 2011, 5:58 a.m.)

   Doesn't make me much difference... seems pretty unlikely anyone would have one and not the other though.

2. Nathan Binkert May 2, 2011, 6:08 a.m. (May 2, 2011, 6:08 a.m.)

   Old repos may not have the new hook. (I only found out that we could use pre-qrefresh a few months ago).

3. Steve Reinhardt May 2, 2011, 7:06 a.m. (May 2, 2011, 7:06 a.m.)

   The only hitch is that if we just append the hook text when only one is missing we'll get duplicates, like this:
   
   [extensions]
   style = /home/stever/hg/amd/m5/util/style.py
   
   [hooks]
   pretxncommit.style = python:style.check_style
   
   # The following lines were automatically added by m5/SConstruct
   # to provide the m5 style-checking hooks
   [extensions]
   style = /home/stever/hg/amd/m5/util/style.py
   
   [hooks]
   pretxncommit.style = python:style.check_style
   pre-qrefresh.style = python:style.check_style
   # End of SConstruct additions
   
   I don't think it's a problem (my very limited testing hasn't shown anything), but it is a little weird.  If you think this is OK, then I'll go ahead and test for both.  Otherwise I could test for both, print a warning if they're only half there, and only do the auto append if they're both missing.
   

4. Nathan Binkert May 2, 2011, 9:48 a.m. (May 2, 2011, 9:48 a.m.)

   I thought about that.  It's probably fine.  A warning is probably worth it.  Though, again, if you don't stop output for the user, nobody will notice it, so maybe it doesn't matter

SConstruct (Diff revision 1)

Should we prompt the user for permission?  That way the user would at least know that something happened.

1. Steve Reinhardt May 2, 2011, 6:01 a.m. (May 2, 2011, 6:01 a.m.)

   I doubt most people care.  How many would really say no?  Most people just want things to be automatic.
   
   If they really do care they can always edit it out later (the comments will let them know where the hooks came from).
   

2. Nathan Binkert May 2, 2011, 6:06 a.m. (May 2, 2011, 6:06 a.m.)

   I agree that nobody would say no, but most people also would never notice that it had happened because the message would just fly by if there was no prompt.  I guess that you're arguing that that would be a good thing :)

3. Gabe Black May 2, 2011, 8:27 a.m. (May 2, 2011, 8:27 a.m.)

   Changing config files behind peoples back is a really bad idea in my opinion. I know I've stopped using entire distros (Suse) because they mucked with config files behind my back and perpetually broke my system. My configs are mine, and the minimal level of respect for that would be if we asked permission before we let ourselves in. I think leaving well enough alone and having the user go in and fix it themselves is actually the best approach.

4. Nathan Binkert May 2, 2011, 9:49 a.m. (May 2, 2011, 9:49 a.m.)

   This isn't a config file in your homedir, it's the config file in the M5 repo and it will only be changed if you don't have it set up right, so it's not all that bad.  I have the same leaning that you do, so I think I'd rather see a prompt for the user, but I don't feel that strongly about it.

5. Steve Reinhardt May 2, 2011, 12:03 p.m. (May 2, 2011, 12:03 p.m.)

   Yea, I would feel differently about ~/.hgrc, but this is just a local config file, and it's one that will need updating each time you clone a new repo.  I don't mind making the warning a little more prominent, but since the status quo is that we won't even try to compile if the hooks aren't there, it seems overkill to me to make too big a deal of it... is someone going to be so offended about this that they're going to go try a different simulator?  Plus if we could find a way to get these hooks to be part of the m5 repo so that they were automatically in place whenever you cloned, we would have done that, so I don't see how this is much different.

6. Gabe Black May 2, 2011, 1:11 p.m. (May 2, 2011, 1:11 p.m.)

   I'm just afraid of this happening, causing some problem, somebody spending a long time and discovering (or not discovering) that some config file was changed without their knowledge and that's what was breaking everything. I can't imagine somebody not being a little miffed about that. But if you think it's really really safe and you've made it obvious what happened then I guess it could be ok. Maybe make it stop scons after that warning prints if those lines aren't there so it's harder to miss? Then they can just run it again and it'll go through, but they at least know what happened.
   
   And yes, I have heard of people that have tried to use M5, got frustrated and gone elsewhere. It is very possible to be too annoying or difficult and to drive away users.

Whether you decide to change this or not, it's a good idea.