Fix Windows CI builds #361
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #361 +/- ##
==========================================
+ Coverage 89.64% 89.69% +0.04%
==========================================
Files 39 39
Lines 5169 5209 +40
==========================================
+ Hits 4634 4672 +38
- Misses 535 537 +2
Continue to review full report at Codecov.
|
duckontheweb
force-pushed
the
windows-ci-fixes
branch
from
d8e7cd3
to
79eacfd
Compare
Actually, I think I have some fixes to most of these. I'll update the PR shortly. |
duckontheweb
force-pushed
the
windows-ci-fixes
branch
from
2cfc0ea
to
9ddc7a9
Compare
- OS-specific dependency caching in GitHub Actions - Separate test requirements for faster builds - Separate lint and test CI jobs
These tests have test cases which are specific to posix paths and will cause failures when run on Windows.
The default encoding on Windows is not necessarily utf-8, which can lead to problems when reading/writing across platforms. This explicitly uses utf-8 for all file encoding & decoding.
The default temp directory in GitHub Actions Windows runners is on the C: drive, but the test code and data files are on the D: drive. This was leading to CI failures due to an error in ntpath when trying to determine relative paths.
This makes the utils for safely parsing and joining URLs "public" and uses them throughout the code base and tests to ensure that we don't use platform-specific separators when it's not appropriate (i.e. for URLs).
Fixes some issues where relative and absolute hrefs were not being constructed correctly for URLs on Windows. Also creates separate internal functions for handling these 2 cases to make the logic more clear.
duckontheweb
force-pushed
the
windows-ci-fixes
branch
from
c92c0df
to
612a737
Compare
…clean # Conflicts: # requirements-dev.txt # scripts/test # tests/extensions/test_label.py # tests/test_catalog.py # tests/test_layout.py # tests/test_utils.py # tests/test_writing.py
l0b0
reviewed
Jun 1, 2021
Goes back to using more specific pip- prefix for dependency caches and doesn't needlessly install package for linting
l0b0
added a commit
to l0b0/pystac
that referenced
this pull request
Jun 3, 2021
Based heavily on <stac-utils#361>.
l0b0
added a commit
to l0b0/pystac
that referenced
this pull request
Jun 3, 2021
Based heavily on <stac-utils#361>.
l0b0
reviewed
Jun 4, 2021
duckontheweb
commented
Jun 4, 2021
lossyrob
approved these changes
Jun 4, 2021
There was a problem hiding this comment.
LGTM. Added a couple of test cases I was curios about in duckontheweb#1, these passed.
duckontheweb
commented
Jun 4, 2021
|
@l0b0 I think I've addressed all of your comments. Let me know what you think. |
LGTM, cheers! |
l0b0
approved these changes
Jun 4, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related Issues:
Description:
Uses
shell: bashto ensure we're running abashshell, even on Windows.The cache keys are now specific to Python version and OS and point to the correct cache directories for each OS.
In the GitHub Actions Windows runner the default
TMPDIRdirectory is on a different drive (C:\\) than the code and test data files (D:\\). This was causing failures inos.path.relpathon Windows.The lint and type checking jobs only need to run once per Python version since they are not platform-specific.
In
pystac.utilsrenames_urlparse->safe_urlparseand_join->join_path_or_urland uses these functions throughout the codebase and tests to ensure we are joining paths properly. There were a number of places whereos.path.joinwas being used to join URL paths, which caused failures on Windows. Also changes the signature ofjoin_path_or_urlto take aJoinTypeenum as the first argument to make the usage more clear.When using functions like
pystac.utils.make_absolute_href, the drive letter was being converted to lowercase. Thesafe_urlparsefunction now preserves the original casing from the start path (see the changes to the tests in 8e9455 for an example)utf-8encoding for all file I/O operationsThe default encoding on Windows is not necessarily UTF-8, which was causing decoding errors when trying to read test data files. This uses
encoding="utf-8"for all file read/write operations to avoid conflictsmake_absolute_hrefandmake_relative_hrefWhen debugging the Windows issues, it was pretty difficult to reason through how a URL would be handled in those functions vs. a local path. This PR creates distinct internal functions to handle each case. The result is some redundancy in the code, but I think the trade-off in clarity is worth it.
UPDATE 2021-06-03
setup.pyto linting and auto-formattingsetup.pytosetup.cfg. This means we no longer have to use the deprecatedimpmodule.PR Checklist:
scripts/format)scripts/test) (see comments above)