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

[bzip2] fixes for building release-only #12759

Closed
wants to merge 6 commits into from

Conversation

cenit
Copy link
Contributor

@cenit cenit commented Aug 5, 2020

Restore the possibility to build bzip2 with a release-only triplet.
I'd also suggest to move one of the CI pipelines to test ports for this configuration, which is very common in production.
Closes #12799

@cenit
Copy link
Contributor Author

cenit commented Aug 5, 2020

EOL is now LF only, sorry for the very noisy diff. Luckily GitHub has an option in the diff view to hide whitespace-only changes

ports/bzip2/portfile.cmake Outdated Show resolved Hide resolved
@LilyWangL LilyWangL added the category:port-bug The issue is with a library, which is something the port should already support label Aug 6, 2020
@LilyWangL
Copy link
Contributor

LilyWangL commented Aug 6, 2020

The error on CI baseline will be fixed in PR #12766.

@LilyWangL LilyWangL added the depends:different-pr This PR or Issue depends on a PR which has been filed label Aug 6, 2020
@LilyWangL LilyWangL removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Aug 7, 2020
@Neumann-A
Copy link
Contributor

@cenit: please stop mixing port fixes with smaller script fixes. Your are invalidating all the cached archives which use vcpkg_fixup_pkgconfig() and don't depend on bzip2. Although you might not see the benefit of message(STATUS "Checking file: ${_file}") since you probably only dealt with ports installing only a few *.pc files it is quite important to know where the error occurred if you have to deal with a port installing 20+ *.pc files

@cenit
Copy link
Contributor Author

cenit commented Aug 7, 2020

That's a problem for port creators, not port users. For the same reasons there is no output for vcpkg_fixup_cmake_targets or many others.
We should assume that when you install the port, it should just work, and output should be minimal as it always has been

@c72578
Copy link
Contributor

c72578 commented Aug 9, 2020

We should assume that when you install the port, it should just work

This PR here already shows the opposite of such an assumption

@PhoebeHui PhoebeHui assigned PhoebeHui and unassigned PhoebeHui Aug 10, 2020
@cenit
Copy link
Contributor Author

cenit commented Aug 10, 2020

This PR here already shows the opposite of such an assumption

again, that's not a reason to fill up user screen with unnecessary debug information.
Such problems should be detected during review and from CI

@Neumann-A
Copy link
Contributor

@cenit with that logic vcpkg should not output anything. The messages are meant as progress messages not debug info. If you would rather like to see 'fixing pkgconfig x of y' I can make that change. I would also like to see some progress messages for fixup_cmake because ports like qt and vtk take a long time to fixup due to the amount of small file io.

@cenit
Copy link
Contributor Author

cenit commented Aug 10, 2020

with that logic vcpkg should not output anything

tbh I'd love it :)
Output only in case of errors, otherwise just a sequence of
"Starting package 1/97"
"Finishing package 1/97"
....
"Starting package 97/97"
"Finishing package 97/97"
such a lovely output 😄

@AenBleidd
Copy link
Contributor

Maybe just a global verbosity flag should be added for vcpkg? Then every port user can control amount of information they'd wish to see...

@cenit
Copy link
Contributor Author

cenit commented Aug 10, 2020

on files (with a fixed standardized syntax) of course you can write as much as you want!

@AenBleidd
Copy link
Contributor

@LilyWangL, any news here? All CI fails looks like not really related to this particular PR...

@past-due
Copy link
Contributor

I've also hit this issue, and am eager to see this fix merged.

@LilyWangL
Copy link
Contributor

The CI fails is not related with this PR, it will be fixed by PR #12856.

@LilyWangL LilyWangL added the depends:different-pr This PR or Issue depends on a PR which has been filed label Aug 12, 2020
@gitcop-devel
Copy link

@cenit, please follow common git commit practices:

Make only one change per commit and avoid including unrelated changes

The changes to the file scripts/cmake/vcpkg_fixup_pkgconfig.cmake are unrelated to "[bzip2] fixes for building release-only" (commit a3e2935)

@AenBleidd
Copy link
Contributor

@gitcop-devel, this repo contains examples that contradict this rule ;)

@cenit cenit closed this Aug 14, 2020
@cenit cenit deleted the dev/cenit/bzip2 branch August 24, 2020 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support depends:different-pr This PR or Issue depends on a PR which has been filed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bzip2] release-only build failure on Windows
9 participants