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

Redoing #61, but as a direct branch. #62

Merged
merged 5 commits into from
Oct 21, 2024
Merged

Redoing #61, but as a direct branch. #62

merged 5 commits into from
Oct 21, 2024

Conversation

nicolasnoble
Copy link
Contributor

To help the CI, I guess.

@mc-nv
Copy link
Contributor

mc-nv commented Oct 17, 2024

May be it make sense to simply install patch command inside Windows container using pip and have no reason to deal with extra logic in new file?

@fpetrini15
Copy link

@nicolasnoble your changes passed CI including the Windows build (CI pipeline ID 19426602).

Let me investigate whether I can get patch installed in the windows container, otherwise, we can get this merged.

@nicolasnoble
Copy link
Contributor Author

You'd need both patch and true on the Windows container, for the original code to work.

@mc-nv
Copy link
Contributor

mc-nv commented Oct 17, 2024

You'd need both patch and true on the Windows container, for the original code to work.

Why we need true statement, can't we use -t flag?

$ patch --help
---
Miscellaneous options:

  -t  --batch  Ask no questions; skip bad-Prereq patches; assume reversed.
  -f  --force  Like -t, but ignore bad-Prereq patches, and assume unreversed.
  -s  --quiet  --silent  Work silently unless an error occurs.
  --verbose  Output extra information about the work being done.

@nicolasnoble
Copy link
Contributor Author

Because unfortunately, that's not what cmake expects. The PATCH_COMMAND has to be idempotent, meaning it should always result in the patch, whatever happens. Using -t would make it work once properly, and then the second time you run a build in the same directory, you'd reverse the patch, as this is what this flag implies.

There is just no squaring this circle: cmake's PATCH_COMMAND and the base patch commands are inherently incompatible in design and behavior.

@mc-nv
Copy link
Contributor

mc-nv commented Oct 17, 2024

Curious, may it better to compile "absl" with or without patch once and feed package to gRPC.

@nicolasnoble
Copy link
Contributor Author

I mean, I was trying to go for the minimum set change here, but what you are suggesting requires a much bigger overhaul of the cmake architecture of Triton, which isn't a yak I'm willing to start shaving.

Feel free to come up with a better pull request for this?

@fpetrini15
Copy link

fpetrini15 commented Oct 18, 2024

This is justification enough for me. Even with the installation of patch I don't believe it would be able to interpret true. This change is benign, speeds up our build process, and will unblock us on the Windows build--so I am moving to approve.

If we really don't like having this extra file in the future, we can work to remove it.

@fpetrini15 fpetrini15 self-requested a review October 18, 2024 16:28
@mc-nv mc-nv self-requested a review October 18, 2024 21:38
@nicolasnoble nicolasnoble merged commit 22c41d7 into main Oct 21, 2024
3 checks passed
@nicolasnoble nicolasnoble deleted the fix-windows branch October 21, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants