Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge Changes From Rob and Richard for ISIS SANS #393

Merged
merged 10 commits into from Apr 1, 2015

Conversation

PeterParker
Copy link
Contributor

Fixes trac issue #11360.

To tester

These are changes given to us by Rob Dalgliesh and Richard Heenan, which I'm merging for them almost completely "as is". I have:

  • given the code a manual inspection to see that it is sensible;
  • fixed a couple of issues that cropped up with the fact that they were using slightly outdated versions of files; and
  • fixed broken system tests.

I am submitting this pull request so that I may provide them both with a build which they can use to give the changes a final "once-over" before the cycle starts this week.

For testing purposes, I'd recommend waiting until we hear back from the scientists that the changes are working as they intended, then simply do a code review.

Rob has said that he would like to provide us with system test data / reduction scripts so that we may have LARMOR covered with adequate test coverage. Having said that, they are very keen on getting something they can use into the nightly builds ASAP, so we will have to do this in a separate ticket at a later date.

The files have moved on from the version of the code used by Rob, so this
required some work to bring in the changes.  System tests are passing so I
think we're okay here.

Refs #11360.
The new params just needed defaults.
It looks like RotateInstrumentComponent does actually rotate even if the
angle is 0.0.  Just dont call it in this case.

Should fix remaining system tests.
@PeterParker PeterParker added High Priority An issue or pull request that if not addressed is severe enough to postponse a release. SANS Issues and pull requests related to SANS In Progress labels Mar 15, 2015
@PeterParker
Copy link
Contributor Author

Win7 build failed with the following:

19:36:54 ApplicationWindow-[Notice] Script execution finished.
19:36:54 MantidUI-[Notice] MantidPlot is shutting down...
19:36:56 C:\Jenkins\workspace\pull_requests\label\win7-build\build
19:36:56 Removing package 'C:\Jenkins\workspace\pull_requests\label\win7-build\build\mantid-3.3.20150315.1903-win64.exe'
19:36:56 Traceback (most recent call last):
19:36:56   File "C:\Jenkins\workspace\pull_requests\label\win7-build\Code\Mantid\Testing\SystemTests\scripts\mantidinstaller.py", line 274, in <module>
19:36:56     installer.uninstall()
19:36:56   File "C:\Jenkins\workspace\pull_requests\label\win7-build\Code\Mantid\Testing\SystemTests\scripts\mantidinstaller.py", line 136, in uninstall
19:36:56     self.do_uninstall()
19:36:56   File "C:\Jenkins\workspace\pull_requests\label\win7-build\Code\Mantid\Testing\SystemTests\scripts\mantidinstaller.py", line 165, in do_uninstall
19:36:56     run('start "Uninstaller" /wait ' + uninstall_path + ' /S')
19:36:56   File "C:\Jenkins\workspace\pull_requests\label\win7-build\Code\Mantid\Testing\SystemTests\scripts\mantidinstaller.py", line 87, in run
19:36:56     raise Exception('Returned with code '+str(p.returncode)+'\n'+out)
19:36:56 Exception: Returned with code 1
19:36:56 Access is denied.

Which looks very much like an intermittent problem with the builder rather than an issue with the changes made as part of this ticket.

@PeterParker
Copy link
Contributor Author

Jenkins, retest this please

1 similar comment
@PeterParker
Copy link
Contributor Author

Jenkins, retest this please

Sophos is complaining about unistall.exe again for some reason.  It's
possible that it's quite flaky about changes to XML files, and the
only XML file that we changed in this branch was the LARMOR param file.

Making a trivial change to a comment to try and motivate a different
result.
…ntidproject/mantid into 11360_merge_robs_larmor_changes
These changes didn't have the desired effect.

For now, making sure that the Win7 build happens on the machine with
on-access virus scanning turned off, so at least we have something to give
to Richard and Rob.
@PeterParker
Copy link
Contributor Author

Jenkins, retest this please.

@PeterParker
Copy link
Contributor Author

This failed again, but now because of RHEL 6 being flaky!. Pushed a revert of the last two commits to set off yet another build.

@Anders-Markvardsen Anders-Markvardsen self-assigned this Apr 1, 2015
Anders-Markvardsen added a commit that referenced this pull request Apr 1, 2015
…anges

Merge Changes From Rob and Richard for ISIS SANS
@Anders-Markvardsen Anders-Markvardsen merged commit f3f480a into master Apr 1, 2015
@Anders-Markvardsen Anders-Markvardsen deleted the 11360_merge_robs_larmor_changes branch April 1, 2015 13:28
@Anders-Markvardsen
Copy link
Member

Looks good to me

@AndreiSavici AndreiSavici modified the milestone: Release 3.4 Apr 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority An issue or pull request that if not addressed is severe enough to postponse a release. SANS Issues and pull requests related to SANS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants