DM-13608: Do not rely on locale when using diff

[gcomoretto]

Not tested locally, need to be verified by the originator of the issue.
@RobertLuptonTheGood can you please verify if it works.

[jhoblitt]

It isn't clear on the ticket exactly what the problem is or why setting LANG resolves it. I'm guessing that diff is responding to the locale rather than curl. Either way, it would be fanatic to add a unit test to https://github.com/lsst/lsst/blob/master/spec/unit/n8l/up2date_check_spec.rb that demonstrates the problem by setting LANG to a value that reproduces the failure.

[RobertLuptonTheGood]

You usually see problems with LANG != C on non-ascii systems. It's always safer to set LANG=C, and we should just go ahead and do so. I don't think it's worth the time to generate a test.

[jhoblitt]

It is rather poor practice to use LANG=C in a unicode world and most linux distros have been defaulting to a utf-8 locale such as en_US.UTF-8 for the last decade or so.

Regardless, a trivial unit test will prevent a future regression.

[RobertLuptonTheGood]

Please write the unit test if it matters to you.

[timj]

We spent a long time trying to write a test that would force diff to not use the word "differ" in the output but we failed. We even rebooted the mac into Italian and explicitly set LANG and LC_ALL and others in bash. Nothing worked. As pointed out by @PaulPrice on Jira, it would be much better if diff returned an exit status if two files differed rather than relying on the text string which may be locale-dependent.

[timj]

@josh, it's not unicode that's the problem, it's that the message from diff can change depending on what language your computer is using.

[RobertLuptonTheGood]

If the message is different then we should fix the code to use an exit status (cmp can do that, can't diff?) rather than setting LANG (or the locale). I imagine that I can get you access to a machine that illustrates the problem.

[jhoblitt]

@timj I completely agree with @PaulPrice's suggestion but that doesn't mean that there should not be a unit test that demonstrates there is no breakage in various locales, which would be useful regardless of how the check is implemented. At present, I don't see any explanation on how to even reproduce the problem. The comment on unicode locales was just an FYI.

[timj]

As I said, we tried to reproduce the problem but couldn't (and after an hour at it I decided it we were wasting our time -- Italian menus were interesting though). If you know what incantation is needed to trigger the issue on travis that would be great.

[timj]

@jhoblitt why is the code checking the string value from that command? I just checked and cat blah | diff - blah2 does set $? to 1 if they differ and 0 if they are the same. Was there a reason you weren't checking $? instead?

[timj]

@jhoblitt can you think of a reason why we can't check $? after running diff and just see if it is non-zero?

[timj]

@gcomoretto the new version is failing the test suite because one of the tests does an exact match on the warning message contents and for some reason the test is no longer triggering the message:

https://travis-ci.org/lsst/lsst/jobs/347425710

[jhoblitt]

@timj I'm not the author of that check, I merely preserved the original logic while trying to chop up the original script into a collection of smaller, hopefully testable, functions. I don't have any objection to testing for the exit status.

[timj]

@jhoblitt can you help out @gcomoretto with the ruby test code that's failing? We are trying to work out whether the ruby test is looking explicitly for "differ" which no longer appears. Thanks.

[jhoblitt]

whitespace?

[gcomoretto]

Sorry, I corrected the missing tab. However I need some support for the ruby script.

[jhoblitt]

Sure, I'll take a look.

[gcomoretto]

Thank you very much

[jhoblitt]

I've added a commit to the PR branch that updates the the rspec unit tests.

[DM-13608] ~/github/lsst $ bundle exec rspec --format doc -e n8l::up2date_check
Run options: include {:full_description=>/n8l::up2date_check/}

n8l::up2date_check
  script matches master
    prints nothing
  script out of sync with master
    prints a non-fatal warning
  unknown error comparing source against master
    prints a non-fatal warning

Finished in 0.38948 seconds (files took 0.10976 seconds to load)
3 examples, 0 failures

Assuming travis passes, this looks ready to merge but the commits probably should be squashed and the original commit message updated.

[jhoblitt]

I rebased the PR on current master after the merge of #81 .

[gcomoretto]

With the help of @timj I squash my commits into one in the rebased branch. @jhoblitt can approve the pull request, if all problems have been sorted out?

[jhoblitt]

LGTM. Thanks for sticking with it.