Skip to content

Conversation

@charles-zablit
Copy link
Contributor

@charles-zablit charles-zablit commented Nov 3, 2025

Refactor the uses of os.path in update-checkout by the pathlib.Path class to avoid string manipulation. This is part of an effort to improve the reliability of `update-checkout on Windows.

@charles-zablit charles-zablit force-pushed the charles-zablit/update-checkout/add-tests branch 2 times, most recently from d7a52fd to 4f72ffa Compare November 4, 2025 12:44
@charles-zablit charles-zablit changed the title [update-checkout] refactor to use pathlib.Path instead of os.path [update-checkout] refactor to use pathlib.Path instead of os.path and add a README Nov 4, 2025
@charles-zablit
Copy link
Contributor Author

@swift-ci please smoke test

1 similar comment
@charles-zablit
Copy link
Contributor Author

@swift-ci please smoke test

@charles-zablit charles-zablit force-pushed the charles-zablit/update-checkout/add-tests branch from 4f72ffa to 9307640 Compare November 7, 2025 11:27
@charles-zablit
Copy link
Contributor Author

@swift-ci please smoke test


def call_quietly(*args, **kwargs):
kwargs["stderr"] = subprocess.STDOUT
kwargs["encoding"] = "utf-8"
Copy link
Contributor

@adrian-prantl adrian-prantl Nov 10, 2025

Choose a reason for hiding this comment

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

why is this part of "delete unused test"?

./swift/utils/update-checkout --clone --scheme release/6.2
```

The command above will use `HTTP` to clone the repositories. Use `--clone-with-ssh` to clone with `SSH` instead:
Copy link
Contributor

Choose a reason for hiding this comment

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

HTTPS?

@adrian-prantl
Copy link
Contributor

Individually these all look good, but this should have really been three separate PRs.

@adrian-prantl adrian-prantl self-requested a review November 10, 2025 21:40
@xavgru12
Copy link

Are you sure you want to use Pathlib? From my experience it is really slow compared to os.path. I did not have any problems with os.path either.

@charles-zablit charles-zablit force-pushed the charles-zablit/update-checkout/add-tests branch from 9307640 to 1d9d003 Compare November 11, 2025 11:58
@charles-zablit charles-zablit changed the title [update-checkout] refactor to use pathlib.Path instead of os.path and add a README [update-checkout] refactor to use pathlib.Path instead of os.path Nov 11, 2025
@charles-zablit
Copy link
Contributor Author

Individually these all look good, but this should have really been three separate PRs.

I ended up splitting the changes into separate PRs instead to simplify failure tracking (if we see failures in CI).

This PR is now only converting os.path to pathlib.Path.

@charles-zablit
Copy link
Contributor Author

Are you sure you want to use Pathlib? From my experience it is really slow compared to os.path. I did not have any problems with os.path either.

The overall goal is to improve update-checkout's reliability on Windows as well as to make it easier to contribute. pathlib.Path helps us achieve that by explicitly typing paths (str is now Path) and by handling canonicalization and other path manipulation operations for us.

Regarding the slowness concerns, update-checkout never creates Path in a loop and we don't use walkdir either. While it is slower, I don't think we would notice any difference for our usecase. I'm partly basing my reasoning off of the findings here.

@charles-zablit
Copy link
Contributor Author

@swift-ci please smoke test

@charles-zablit
Copy link
Contributor Author

@swift-ci please test windows

1 similar comment
@charles-zablit
Copy link
Contributor Author

@swift-ci please test windows

@charles-zablit charles-zablit merged commit 4c977c6 into swiftlang:main Nov 13, 2025
3 checks passed
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.

3 participants