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

Add new ways to report build failures and giving hint messages from portfile.cmake #1016

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

autoantwort
Copy link
Contributor

@autoantwort autoantwort commented Apr 11, 2023

Motivation 1: In cases like microsoft/vcpkg#30744 users should not create issues (problem was insufficient disk space). In vcpkg_execute_build_process we could add a call to vcpkg_fail_with_user_interaction_required with a proper error message saying the disk is full.

Motivation 2: increase visibility or warnings since users report issues like this: microsoft/vcpkg#30592 (comment) (Could not find make. Please install it through your package manager.) => vcpkg_fail_with_user_interaction_required. Another example would be microsoft/vcpkg#31276 or microsoft/vcpkg#31312 or microsoft/vcpkg#35151 or microsoft/vcpkg#36541 or microsoft/vcpkg#35991 or microsoft/vcpkg#36011

Motivation 3: Many ports emits warnings like the following

alsa currently requires the following libraries from the system package manager:
    autoconf libtool
These can be installed on Ubuntu systems via sudo apt install autoconf libtool

This is confusing if the libs are already installed and the build is successful. These messages should only be printed when the build fails and they should be printed after the "how to report an issue" block to be more visible. => vcpkg_user_hint_on_failure

Also see docs PR microsoft/vcpkg-docs#60

result.user_required_interaction = fs.read_contents(user_required_path, VCPKG_LINE_INFO);
}
const auto user_hints_path = buildpath / Strings::concat("user-hints-", action.spec.triplet(), ".txt");
if (fs.exists(user_hints_path, VCPKG_LINE_INFO))
Copy link
Contributor

Choose a reason for hiding this comment

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

Better make sure that it's a regular file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it is only an internal interface I think it is ok here. Crashing in case of an folder is a good thing. And I don't care if someone wants to use symbolic links in the future.

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Apr 15, 2023

First, I strongly agree we need to improve these cases. Better build error messages for users has tons of potential. I have a few different areas I wanted to comment on, so apologies in advance for the length.

Restating Cases

Listing out some more error cases that the user can fix, along with the diagnostic difficulty:

  • Missing system libraries (hard)
  • Permissions (hard -- looks the same as other issues?)
  • MAX_PATH (medium)
  • Out of Disk (easier)
  • Out of Memory (easier)
  • Missing Windows SDK (easier)
  • Compiler issues (easier?)

Under this lens, I think your proposed mechanisms are intended to handle:

  • Difficult to diagnose / specific-to-port (fail-hints)
  • Easy to diagnose and generic (fail-require-user-interaction)

Instead of separating these two cases entirely, what about only having hints and, if hints are provided, reduce the prominence of the issue message?

Implementation: Port vs Tool

I see a design decision between putting the port in charge versus creating heuristics in the vcpkg tool. Port-based approaches are more easily iterated on, but are also very limiting. It is quite painful to try to regex through build logs from an "error handler" inside CMake (as we've done for some things). On the other hand, in the tool itself progress is slower and it often wouldn't be "worth it" to handle highly exceptional cases.

A bonus of implementing things in the tool is that they get localized :).

Port-approach Communication Channel

I'm not convinced that raw text known-paths in buildtrees/ are the right extension interface. Some alternatives to consider in no particular order:

Fortunately all these should be retrofit-able underneath a CMake function call interface (as you've implemented).

@autoantwort
Copy link
Contributor Author

autoantwort commented Apr 18, 2023

Under this lens, I think your proposed mechanisms are intended to handle:

  • Difficult to diagnose / specific-to-port (fail-hints)
  • Easy to diagnose and generic (fail-require-user-interaction)

Probably I should not hide the description in the docs PR: microsoft/vcpkg-docs#60

The motivation/difference of these two functions is the following:

  • vcpkg_fail_with_user_interaction_required: We are 100% sure what the error is and the user has to fix it. Like disk full, missing windows SDK or missing program that the user must install. In this case we should not explain how to report an issue.
  • vcpkg_user_hint_on_failure: We don't know if the hint resolves the error and maybe it is vcpkg's fault. Like "please install xyz from the system package manager". We don't check if xyz is already installed or not so installing xyz might resolve the issue or xyz is already installed and the problem is something different. So we should still show the explanation on how to report an issue.

@JavierMatosD JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone from the vcpkg team to take a further look. label Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires:vcpkg-team-review This PR or issue requires someone from the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants