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.

Sign up for GitHub

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

Jump to bottom

Initial changes to build on Windows #1295

Merged
merged 5 commits into from
Jul 17, 2024
Merged

Initial changes to build on Windows #1295

merged 5 commits into from
Jul 17, 2024

Conversation

thedataking
Copy link
Collaborator

@thedataking thedataking commented Jul 9, 2024 •
edited
Loading

DONE:

  • fix build with assembly enabled.
  • Clean up cfged uses
  • Add GitHub Actions workflow

Deferred to follow up PR:

  • Enable caching in workflow
  • Run tests in workflow

@thedataking thedataking requested a review from kkysen July 9, 2024 18:58
@thedataking thedataking force-pushed the perl/windows-build branch 4 times, most recently from 076a0e0 to 6a650f1 Compare July 9, 2024 19:21
kkysen
kkysen reviewed Jul 9, 2024
View reviewed changes
tests/seek_stress.rs Outdated Show resolved Hide resolved
@thedataking thedataking force-pushed the perl/windows-build branch 6 times, most recently from b72409a to 3b6e0ad Compare July 10, 2024 23:38
@thedataking thedataking marked this pull request as ready for review July 10, 2024 23:41
@rinon
Copy link
Collaborator

rinon commented Jul 11, 2024

Can you rebase this on top of #1307 ? Removing snprintf is non-trivial, so maybe we should just keep that part for now. Unfortunately the CLI allows dynamic filename formatting with C-style format strings. Maybe we can use https://crates.io/crates/sprintf or similar at some point? We should split the CLI tools into a separate crate in order to not add an extra dependency for the library in that case.

@thedataking thedataking requested a review from kkysen July 11, 2024 23:30
kkysen
kkysen requested changes Jul 12, 2024
View reviewed changes
Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

For the #[cfg] imports, it's definitely cleaner when separate, but these are the tools, so I don't really care. I do think the best would be to have the imports not #[cfg]ed and export from compat platform-independent interfaces (e.x. pub use libc::snprintf in compat::stdio), but again it's the tools, so it's probably not important.

I had a few other little comments, though, and could you rebase it on #1308 (or just wait for that to merge, probably tomorrow)?

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
build.rs Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
tools/compat/errno.rs Outdated Show resolved Hide resolved
tools/compat/getopt.rs Outdated Show resolved Hide resolved
@thedataking
Copy link
Collaborator Author

thedataking commented Jul 13, 2024 •
edited
Loading

I do think the best would be to have the imports not #[cfg]ed and export from compat platform-independent interfaces (e.x. pub use libc::snprintf in compat::stdio), but again it's the tools, so it's probably not important.

Hadn't thought of that; makes it easier to rip out when we get rid of snprintf and other gunk down the line.

@thedataking thedataking requested a review from kkysen July 13, 2024 01:19
rinon
rinon approved these changes Jul 13, 2024
View reviewed changes
Copy link
Collaborator

@rinon rinon left a comment

Choose a reason for hiding this comment

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

A few small nitpicks, but if you deem them unworthy of fixing, this LGTM. Builds fine on my windows 11 machine.

tools/compat/getopt.rs Outdated Show resolved Hide resolved
tools/dav1d.rs Outdated Show resolved Hide resolved
tools/seek_stress.rs Outdated Show resolved Hide resolved
tools/input/annexb.rs Outdated Show resolved Hide resolved
tools/input/ivf.rs Outdated Show resolved Hide resolved
kkysen
kkysen approved these changes Jul 13, 2024
View reviewed changes
Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

LGTM besides Stephen's comments.

@thedataking thedataking force-pushed the perl/windows-build branch 2 times, most recently from 9d94d85 to 6221724 Compare July 17, 2024 00:28
@thedataking thedataking merged commit 74f485b into main Jul 17, 2024
28 checks passed
@kkysen kkysen mentioned this pull request Jul 17, 2024
Merged
kkysen added a commit that referenced this pull request Oct 31, 2024
@kkysen
build.rs: Fix windows cross compilation (#1323)
b8715b6 
@thedataking, I only noticed after you merged #1295 a few things.

This should let us cross-compile from windows, cross-compile to
`*-windows-gnu` (I think; the linker wasn't installed so I couldn't test
all the way), and print a clear error when trying to cross-compile to
`*-windows-msvc`. It also lets us do full cross-compilation on `rav1d`,
as the `snprintf` stuff for windows is only for `rav1d-cli`.

* Fixes #1365.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkysen kkysen kkysen approved these changes

@rinon rinon rinon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thedataking @rinon @kkysen