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

Build without sandboxing on Windows #1485

Merged
merged 5 commits into from
Aug 30, 2024
Merged

Build without sandboxing on Windows #1485

merged 5 commits into from
Aug 30, 2024

Conversation

kylewillmon
Copy link
Contributor

With this patch, all sandboxing code is guarded by #[cfg(unix)].

@kylewillmon kylewillmon requested a review from a team as a code owner August 29, 2024 21:42
@kylewillmon kylewillmon requested review from maxrake and cd-work August 29, 2024 21:42
Copy link
Contributor

@cd-work cd-work left a comment

Choose a reason for hiding this comment

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

I like the choice of making this a platform-specific thing rather than a feature, makes it more about not being possible rather than making it sound like sandboxing is a feature you might want to disable in certain scenarios.

I've got one minor nit but I'm fine with it going either way.

lockfile_generator/src/lib.rs Outdated Show resolved Hide resolved
@maxrake
Copy link
Contributor

maxrake commented Aug 29, 2024

I'm still reviewing...but there are three warnings on the Windows job. Do those need to be accounted for here? If so, then enabling warnings as errors should also be pursued.

@kylewillmon
Copy link
Contributor Author

Do those need to be accounted for here?

They're pretty simple to handle, so sure.

If so, then enabling warnings as errors should also be pursued.

This might just anger people... So I'd rather not do that for now.

@kylewillmon kylewillmon requested a review from cd-work August 29, 2024 22:01
Cargo.lock Show resolved Hide resolved
lockfile_generator/src/lib.rs Outdated Show resolved Hide resolved
@maxrake
Copy link
Contributor

maxrake commented Aug 29, 2024

If so, then enabling warnings as errors should also be pursued.

This might just anger people... So I'd rather not do that for now.

Roger that. Maybe case in point...there are two new warnings now.

Copy link
Contributor

@maxrake maxrake left a comment

Choose a reason for hiding this comment

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

LGTM

@maxrake maxrake self-requested a review August 29, 2024 22:14
Copy link
Contributor

@maxrake maxrake left a comment

Choose a reason for hiding this comment

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

One change requested.

.github/workflows/test.yml Show resolved Hide resolved
cd-work
cd-work previously approved these changes Aug 29, 2024
Copy link
Contributor

@maxrake maxrake left a comment

Choose a reason for hiding this comment

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

LGTM

@kylewillmon kylewillmon merged commit 56496cd into main Aug 30, 2024
15 checks passed
@kylewillmon kylewillmon deleted the unix-only-sandbox branch August 30, 2024 15:06
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