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

feat: clean up path handling #222

Merged
merged 1 commit into from
Sep 7, 2023
Merged

feat: clean up path handling #222

merged 1 commit into from
Sep 7, 2023

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Sep 6, 2023

  • Depend on camino to avoid needless conversions of the camino paths cargo_metadata gives us
  • Add a StoredPath(Buf) type that is for paths that will get written to output files like main.wxs
    • Most interesting part is StoredPathBuf::from_utf8_path
  • Clean up code to use new path types more

@Gankra
Copy link
Contributor Author

Gankra commented Sep 6, 2023

Currently a draft because I want to do some more testing that this approach is sane

@Gankra
Copy link
Contributor Author

Gankra commented Sep 6, 2023

Experimentally turned on unit test CI for linux/macos, working on enabling more of the tests that make sense (it seems like the initialize::execution unit tests are messed up -- they use set_current_dir but weren't using serial_test, and even if they do every other test is racing them. No idea why this isn't a problem on windows...)

* Depend on camino to avoid needless conversions of the camino paths cargo_metadata gives us
* Add a StoredPath(Buf) type that is for paths that will get written to output files like main.wxs
  * Most interesting part is StoredPathBuf::from_utf8_path
* Clean up code to use new path types more
@Gankra Gankra marked this pull request as ready for review September 6, 2023 22:08
@Gankra
Copy link
Contributor Author

Gankra commented Sep 6, 2023

@volks73 I'm now reasonably confident in the implementation, and have everything except for create execution tests being tested on all 3 platforms, which is the functionality one would expect to work cross-platform. Note that this change largely doesn't affect actual behaviour on windows, so it's mostly just trying to make other platforms behave more like windows (ostensibly desirable).

@Gankra
Copy link
Contributor Author

Gankra commented Sep 6, 2023

(additional evidence: i pointed the cargo-dist integration PR to this branch and now the snapshots are the same on all platforms -- without me having to regenerate them, so the windows ones never changed)

axodotdev/cargo-dist#370

Copy link
Owner

@volks73 volks73 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.

@volks73 volks73 merged commit ea3f6a0 into volks73:main Sep 7, 2023
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.

None yet

2 participants