Pull requests

#49 Declined
Repository
dirkbaechle dirkbaechle
Branch
default
Repository
scons scons
Branch
default

Fixes for tests under Windows (bug2872 "Fix tests on Buildbot slaves")

Author
  1. dirkbaechle
Reviewers
Description

Hi,

this is another set of changes and fixes for bug2872. I extended the must_contain* methods of the test framework, such that they all support user-defined find/search functions.

Regards,

Dirk

Comments (17)

  1. dirkbaechle author

    Good catch Anatoly, thanks a lot for reviewing this. The additional "sys import" statements were leftovers from a refactoring...I corrected this now. All remaining imports should really be needed...

  2. William Deegan

    I see a lot of these:

     _envprop = ""
    if sys.platform == "win32":
        _envprop = "ENV=os.environ"
    

    Why is it necessary to do that? Could you extract it out to a function in QMTest/TestCommon.py or TestSCons.py?

  3. dirkbaechle author

    It's necessary because the executables like "latex" or "dvipdf" wouldn't be found under Windows without it. A problem with putting these two lines into its own function is that sometimes a ", " has to get appended to the string, depending on whether more arguments follow or not.

  4. Gary Oberbrunner

    I see how these changes make the tests pass on Windows, but I'd like Rob to review before we pull, since he's the LaTeX guy. Specifically the envprop stuff; if Windows can't find LaTeX without it, shouldn't the tool itself be fixed? Also the dvipdfm change, and the latex flags removal, probably other things. I'll email him specifically about this. Dirk, if you were to break this up into two sets of changes, one for LaTeX and one for all the other stuff, we might be able to pull the other stuff more quickly.

    I do like the test enhancements though, all new flexibility there is much appreciated.

  5. dirkbaechle author

    I understand everybody's concerns about those envprop changes, but for now it's the best I could come up with for the underlying problem: the LaTeX Tool tries to find an executable like "latex" in the system. This could be installed in "C:\emtex" or "C:\miktex" or "D:\texlive", whatever. In order to find it during the initialization of the Tool, its path must be present in the $PATH variable. Note, how this scheme comes up for every live test...it usually isn't such an issue under Linux because most applications are linked to /usr/bin or /usr/local/bin, which are in the internal default paths for the where_is/detect functions.

    About splitting things up into two (or even more) separate pull requests: that would be fine with me. But I'd also like to hear first what Rob says about this. Maybe he really wants to do some changes by himself...just give me a list of files that should be delayed for now, and I'll update the request accordingly.

  6. SCons repo owner

    I think it's reasonable to search the default install locations of the paths in the tools.

    But, also we should provide a way such that tests can pick up an alternative search PATH. (This would allow installing multiple tools in non standard locations, and then adding them the the test PATH in different combinations to verify the preferred tool(s) are picked up by tool initialization).

    Seems like the best place to do this would be in the test framework and have it generate the env=Environment(...) statements for the test SConstructs?

  7. Rob Managan

    A few comments on these patches. I sympathize on your issues with SCons not finding the TeX applications. I had the same problem on windows and had to add a patch to Platform/darwin.py once I found where the paths for them were stored. I would love to think there was a way to get that information out of the Windows repository but I have no idea.

    In dryrun.py I am wondering if you have considered whether "expect" would ever need a different drive letter than D. I remember adding that to the command string to allow building on a different drive...

    Hmm, looks like the extra os.environ['PATH'] include in the Env of glossary.py could be removed now like you did in multirun.py

  8. dirkbaechle author

    Rob, thanks a lot for your input on this. I corrected glossary.py and removed the additional ENV PATH argument. About dryrun.py: there is no drive letter involved actually. Do you mean the "(/D )*" for catching a "cd . & ..." under Linux, as well as a "cd /D . & ..." under Windows (note the forward slash in front of the D)?

    Looking at the comments above, I get the impression that we are all not very happy with the hacks that have to be made (ENV PATH) in order to get the tests working cross-platform. My idea, and this is what I suggest now, would be to go with this patch as it is now, and then add a new SEP (or a simple bug) for general improvements of the Tool subsystem to the Wiki. There we could continue the discussion about new features...

  9. Rob Managan

    Dirk, I was just wanting to make sure that on Windows we catch situations where the output would be "cd /E . & ..." or some other drive letter than D due to an odd configuration of disks.

    I agree that on the long term we need to work on what path to automatically use in SCons. I know the default is to not include everything in the environment.

    I also want to mention in this forum that there are two methods available to check for the presence of a tool. The tests seem to always use test.where_is which searches the whole path defined on the system. test.detect (there maybe a capital in there) only looks in the path that SCons is using. The way they set my Mac up at work the where_is finds many tools that don't actually run on my hardware so I get a lot of test failures. I hope I have accurately described the two methods.

    So I was wondering if we should consider switching over to detect from where_is. I admit it may be a case by case decision for a later time.

  10. dirkbaechle author

    There will never appear a "/E", the "/D" is used to allow changing the drive letter under Windows. Please check SCons/Tool/tex.py at ll. 927.

  11. Rob Managan

    Thanks for reminding me. I thought it had been left as an entry in the environment that the user could change but I guess I never got that done. I could have sworn I did that because some user had his files on a different disk than SCons... or something like that.

  12. dirkbaechle author

    I split up the changes into three parts now. This is the first one and it mainly contains the extensions to the test framework. The MinGW and LaTeX patches each follow in a different pull request.

  13. Gary Oberbrunner

    Hi Dirk, this is fine now. I'd like to pull this, but as one single change if you don't mind, without all the history from your side which I think just clutters it up. I think it stands alone very well. In order for you to get credit for it, could you just make it as a single change from the current tip? (I can do it if you prefer.)

    One minor thing to consider: should your search_re (used in a couple of tests) be pulled into the TestCommon system?