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

[vcpkg_download_distfile] always display output for downloading #40804

Closed
wants to merge 2 commits into from

Conversation

konjac
Copy link

@konjac konjac commented Sep 4, 2024

This is for refinement mentioned in #40810

In vcpkg_download_distfile.cmake, it always hides the output of package downloading via x-download unless a failure. This behavior is not so friendly for debugging scenarios which need output. For example, when I configured asset cache with an external tool, I want to know whether the cache is hit.

So I propose this change to always show that output.

Before the change

image

After the change

image

@konjac konjac changed the title [vcpkg_download_distfile] always display download output [vcpkg_download_distfile] always display output for downloading Sep 5, 2024
@FrankXie05 FrankXie05 added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Sep 5, 2024
@JonLiu1993 JonLiu1993 linked an issue Sep 5, 2024 that may be closed by this pull request
@konjac
Copy link
Author

konjac commented Sep 6, 2024

@FrankXie05 The pr build is failing but seems unrelated to my change. Could you help check or retry the build?

@FrankXie05
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Sep 11, 2024
@@ -265,8 +265,9 @@ If you do not know the SHA512, add it as 'SHA512 0' and re-run this command.")
WORKING_DIRECTORY "${DOWNLOADS}"
)

message("${output}")
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better instead to just not redirect output in the first place? (That is, remove OUTPUT_VARIABLE and ERROR_VARIABLE above entirely rather than reprinting it here?)

Should we get rid of the message call on line 239 now?

Copy link
Author

Choose a reason for hiding this comment

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

@BillyONeal Thanks for the suggestion.

I think line 239 is still necessary and we can enclose the output like this, which could be better.

Suggested change
message("${output}")
if((NOT vcpkg_download_distfile_QUIET) OR (NOT "${error_code}" EQUAL "0"))
message("${output}")
endif()

After such a change, the log output is displayed when vcpkg_download_distfile_QUIET is off or error code is not zero.

Copy link
Member

Choose a reason for hiding this comment

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

After such a change, the log output is displayed when vcpkg_download_distfile_QUIET is off or error code is not zero.

Isn't the whole point of this change that we always want to display the output, even when the exit code is zero?

@BillyONeal
Copy link
Member

The --debug on line 260 seems wrong.

@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Sep 11, 2024
@JavierMatosD
Copy link
Contributor

JavierMatosD commented Sep 12, 2024

Thank you @konjac for your contribution. I've chosen to close this PR in favor of #40945

@konjac
Copy link
Author

konjac commented Sep 14, 2024

Thank you @konjac for your contribution. I've chosen to close this PR in favor of #40945

I think it is very impolite to close my PR without any prior requests. And actually, your new PR is just a few changes based on my idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vcpkg_download_distfile] always display output for downloading
4 participants