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

Better cross-platform compatibility in test files #444

Merged
merged 5 commits into from
Aug 5, 2020
Merged

Better cross-platform compatibility in test files #444

merged 5 commits into from
Aug 5, 2020

Conversation

t9t
Copy link
Contributor

@t9t t9t commented Aug 3, 2020

This is a (small) step towards #347, to ensure tests run on Windows. I have found a pretty good way to run the tests both on Linux and Windows (manually, yet) which allows me to identify areas which need some more work, address them, then check if tests still run on both.

  • os.getenvb is not supported on Windows
  • rm -rf obviously also isn't, unfortunately shutil.rmtree only removes directories so an additional check to delete files was necessary
  • Again, obviously no /dev/urandom on Windows

Copy link
Member

@ericzolf ericzolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just a few changes necessary. Thanks a lot.

testing/librsynctest.py Outdated Show resolved Hide resolved
testing/commontest.py Outdated Show resolved Hide resolved
@ericzolf
Copy link
Member

ericzolf commented Aug 4, 2020

I didn't realise but what you described in #443 also happens in the Travis CI, meaning we can't merge even if you fix the few review comments. We need first to fix the Travis pipeline. Something has changed in the environment, it used to work for months. Let me know if you want to fix it yourself (even in this same PR) or if I should do it.

@t9t
Copy link
Contributor Author

t9t commented Aug 4, 2020

Thanks for the pointers, I really appreciate it! I will address them later today. I can also fix the issue described in #443 in this PR, no problem.

When you have some time, I am interested to hear how to run individual tests outside of tox. Being still largely unfamiliar with this ecosystem of tools, I've now resorted to running tox from PyCharm, which is less than ideal.

@t9t
Copy link
Contributor Author

t9t commented Aug 4, 2020

Looks like "later today" is now. :) I have addressed the pointers. I also changed the Travis Windows build command but it seems like it didn't pick it up for the pull request build..., that is a bit awkward. :) https://travis-ci.com/github/rdiff-backup/rdiff-backup/jobs/368143319

@ericzolf
Copy link
Member

ericzolf commented Aug 4, 2020

Run individual tests outside of tox: check the docs/DEVELOP.md where I explain how to debug, and just remove the dbg/-pydebug things and then it works also with tests. Shortly summarized:

./setup.py clean --all  # only if you've built wheels before and the shell complains about missing python
./setup.py build
PATH=$PWD/build/scripts-3.7:$PATH PYTHONPATH=$PWD/build/lib.linux-x86_64-3.7 python testing/somethingtest.py

The nice thing is that you can also use the unittest parameters e.g. --failfast. It works similarly under Windows. If you peek again at my ansible playbook tools/windows/playbook-build-rdiff-backup.yml, the 2 last tasks, you'll see that I copy around the rsync.dll and create a bash file to export the necessary variables.

@ericzolf
Copy link
Member

ericzolf commented Aug 4, 2020

Regarding the Travis error, it seems to take the new version (I see the added -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=TRUE option in the job logs) but to not be enough to succeed. No clue what changed, I need to check in my own environment if it still works and what there are for other differences.

Do you have enough rights to see the Travis job logs?

@t9t
Copy link
Contributor Author

t9t commented Aug 4, 2020

@ericzolf Thanks for the testing instructions, I will check it out!

As for the Travis, yeah I didn't look closely enough and was looking only at the abbreviation. Upon closer inspection indeed it seems to have taken the changed command line.

I did some permutations on my machine (because honestly I have no knowledge about cmake at all...) and turns it out it's rather simple. There was -A Win32 but the build runs on an AMD64 machine in Travis. So now I changed it to -A x64 (and reverted the other settings) to see if that will work.

Do you have enough rights to see the Travis job logs?

Yes!

@t9t t9t requested a review from ericzolf August 4, 2020 12:13
@ericzolf
Copy link
Member

ericzolf commented Aug 5, 2020

Everything looks good so nothing to do with your changes but I'm still hesitating:

  1. it used to work with Win32, why doesn't it any more?
  2. as we setup the automated build, we explicitly set Win32 because user voices stated that they need the Windows binary to be 32 bits and not only 64...
  3. I now saw that under tools/windows, I already use x64 but that's just my development environment, not what we deliver.

@t9t
Copy link
Contributor Author

t9t commented Aug 5, 2020

Yeah I totally understand. What I observed is that using -A Win32 with cmake didn't work on my 64 bit computer, nor in the Travis build pipeline for this pull request, but that it does work with -A x64. Unfortunately I have no knowledge of cmake so I can't make any judgment about whether it should work or not. Perhaps they did an update which broke this (seems unlikely to me, though).

At the same time I'm fairly certain that my code changes did not break the pipeline either, as it already didn't work on my computer before I started tinkering with the code (hence why I raised #443).

Is there any way to start a Travis CI build for master and see if that works just fine?

I would propose that I strip the commits that are trying to fix the Travis build from this pull request and limit the scope to only the changes in the Python files. Then the Travis build issue can be addressed separately and doesn't have to interfere with these other changes.

@t9t
Copy link
Contributor Author

t9t commented Aug 5, 2020

This is the last build on master: https://travis-ci.com/github/rdiff-backup/rdiff-backup/jobs/366370770
This is the first build on my branch: https://travis-ci.com/github/rdiff-backup/rdiff-backup/jobs/368011823

When quickly scanning some of the output, I notice the following:

  • master: creating build\lib.win32-3.7
  • my branch: creating build\lib.win-amd64-3.7

So it looks like somewhere either between 7 days ago and now, or between master and my branch, it has changed between using win32 to win-amd64. I can imagine this is also the culprit why it can't find the symbols, as the librsync was built for Win32, but now it's trying to find an x64 library or something. I believe this is something in the Python Setuptools which is building the extension, but then again my knowledge of this ecosystem is so limited that this is just a wild guess.

Doing some experimentation locally again, it looks like it takes this value from the currently used Python installation. When I run python setup.py build using a 64-bit Python, it will use win-amd64 and it will fail when librync was built using -A Win32. On the other hand, when I run python setup.py build explicitly using a 32-bit Python, it will work fine.

Checking now how Python is installed in Travis, it seems like this is the issue: pyenv-win/pyenv-win#128
pyenv-win now by default installs a 64-bit Python rather than 32-bit. And since rdiff-backup just takes the master rather than any specific tag or release, any newly started build will get such changes for free.

I will try and see how difficult it is to pin a pyenv-win version in the Travis configuration so that until further changes it will just take the version that was running fine, before that change.

That situation is addressed in: #445.

I have removed the commits that changed that Travis cmake command from this pull request.

@ericzolf
Copy link
Member

ericzolf commented Aug 5, 2020

I've merged back master into this branch so there shouldn't be anything against merging if the tests don't fail again.

@ericzolf ericzolf merged commit ceb3432 into rdiff-backup:master Aug 5, 2020
@ericzolf
Copy link
Member

ericzolf commented Aug 5, 2020

It was a bumpy ride for a first PR (which became a 2nd one)! Thanks for your tenacity!

@t9t t9t deleted the crossplatform1 branch August 7, 2020 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants