Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make --only-downloads not affect ABI #1363

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

BillyONeal
Copy link
Member

While reviewing changes to #1339 , I observed that it was strange that the abi hash depends on whether download_only is selected. After discussion with @ras0219-msft , he agreed that the ABI hash shouldn't be affected by this setting.

However, given that in download mode, we intentionally build things which do not have all their dependencies present, it would not be safe to attempt to cache or otherwise install anything in that condition.

This change removes download_only from the ABI calculation, and changes installation to only attempt to install built bits whose dependencies are all satisfied.

…range that the abi hash depends on whether download_only is selected. After discussion with @ras0219-msft , he agreed that the ABI hash shouldn't be affected by this setting.

However, given that in download mode, we intentionally build things which do not have all their dependencies present, it would not be safe to attempt to cache or otherwise install anything in that condition.

This change removes download_only from the ABI calculation, and changes installation to only attempt to install built bits whose dependencies are all satisfied.
@dg0yt
Copy link
Contributor

dg0yt commented Mar 9, 2024

changes installation to only attempt to install built bits whose dependencies are all satisfied.

IIUC this means that --only-downloads will omit many asset downloads which are actually expected.

IMO the goal is not to skip installations (which implement the downloads) but artifact caching (like --editable or --no-binarycaching?). The original ABI hash impact didn't prevent an upload, but moved it to a pointless ABI hash.

@BillyONeal
Copy link
Member Author

IIUC this means that --only-downloads will omit many asset downloads which are actually expected.

We still run their builds, just not the resulting installs. Ports that provide common helpers like vcpkg-cmake are at the bottom of the tree, so all their dependencies will be satisfied.

IMO the goal is not to skip installations (which implement the downloads) but artifact caching (like --editable or --no-binarycaching?).

No, installations need to be skipped. Consider the following:

Port A has:

find_package(PortB) # optional
if(NOT PortB)
    # do nothing
endif()

but we think this is OK because in Port A's vcpkg.json, they say:

{
    "dependencies": ["portb"]
}

But port B has an execute_process somewhere, so building it fails in download mode (after downloading its bits).

Before this change, vcpkg install porta --only-downloads ends up 'succeeding' in 'installing' Port A, but the resulting installation is broken. If there's a port C which depends on port A, and you try to install that without --only-downloads, your port C is broken, because port A looks installed! It's just broken.

After this change, we still build ports A and B, but we don't install the broken port A. Note that we still build it, just like before, meaning its downloads still happen. We just don't do the packages->installed part.

It is possible that this will fail to download some stuff if there's something that's not at the bottom of the tree which provides helpers which download things whose dependencies are unsatisfied. But as far as I know we don't have any of those....

@BillyONeal BillyONeal merged commit fa20b09 into microsoft:main Mar 12, 2024
5 checks passed
@BillyONeal BillyONeal deleted the download-only-abi branch March 12, 2024 19:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants