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

#1648 building dependencies with vcpkg (skip ci) #1695

Closed
wants to merge 1 commit into from

Conversation

CAMOBAP
Copy link
Contributor

@CAMOBAP CAMOBAP commented Nov 12, 2021

@CAMOBAP CAMOBAP self-assigned this Nov 12, 2021
@CAMOBAP CAMOBAP force-pushed the feature/use-vcpkg-for-all-platform branch 18 times, most recently from 1d0086e to d5c805d Compare November 18, 2021 19:02
@CAMOBAP CAMOBAP force-pushed the feature/use-vcpkg-for-all-platform branch 5 times, most recently from 9ec4d21 to 93da442 Compare November 20, 2021 13:30
@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Nov 20, 2021

@ni4 @ribose-jeffreylau @maxirmx ubuntu is done, I plan to move forward to other platforms, I would like to get feedback on this moment, to make sure that I didn't break something important

Please don't hesitate to share your questions/feedback

@ronaldtse
Copy link
Contributor

Thanks @CAMOBAP , I think this looks reasonable.

@maxirmx
Copy link
Member

maxirmx commented Nov 21, 2021

@CAMOBAP
Looks good for me though I have a note.

You now have both

 USE_STATIC_DEPENDENCIES: yes

and

triplet: [x64-linux, x64-linux-dynamic]

and it looks like some logical inconsistency. I guess USE_STATIC_DEPENDENCIES does not have any sence now and can be removed.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Nov 21, 2021

@maxirmx thanks for the feedback. I also was surprised that USE_STATIC_DEPENDENCIES affects non-cacheable dependencies.

https://github.com/rnpgp/rnp/blob/master/ci/lib/install_functions.inc.sh#L276-L280

In my opinion USE_STATIC_DEPENDENCIES should not affect non-cacheable dependencies, how do you think?

@CAMOBAP CAMOBAP force-pushed the feature/use-vcpkg-for-all-platform branch 8 times, most recently from 7de0cc5 to 02ad807 Compare November 21, 2021 15:32
@maxirmx
Copy link
Member

maxirmx commented Nov 21, 2021

@maxirmx thanks for the feedback. I also was surprised that USE_STATIC_DEPENDENCIES affects non-cacheable dependencies.

https://github.com/rnpgp/rnp/blob/master/ci/lib/install_functions.inc.sh#L276-L280

In my opinion USE_STATIC_DEPENDENCIES should not affect non-cacheable dependencies, how do you think?

I think it should not be a restriction as it appears to be. IMHO it shall be possible to use both sfatic and dynamic deoending on the application context.

@ribose-jeffreylau
Copy link
Contributor

@maxirmx thanks for the feedback. I also was surprised that USE_STATIC_DEPENDENCIES affects non-cacheable dependencies.

https://github.com/rnpgp/rnp/blob/master/ci/lib/install_functions.inc.sh#L276-L280

In my opinion USE_STATIC_DEPENDENCIES should not affect non-cacheable dependencies, how do you think?

To provide some context, I think I grouped asciidoc and bundler under "non-cacheable" as I wasn't sure how to properly cache their installations. I believe that to be the only reason.

@CAMOBAP CAMOBAP force-pushed the feature/use-vcpkg-for-all-platform branch 4 times, most recently from c8f56b0 to e80abaa Compare November 23, 2021 18:30
@CAMOBAP CAMOBAP force-pushed the feature/use-vcpkg-for-all-platform branch from e80abaa to 375e165 Compare November 24, 2021 10:10
@ni4
Copy link
Contributor

ni4 commented Dec 7, 2022

@maxirmx @ribose-jeffreylau Does this PR have any value at this moment or we could safely close it now?

@maxirmx
Copy link
Member

maxirmx commented Dec 7, 2022

@maxirmx @ribose-jeffreylau Does this PR have any value at this moment or we could safely close it now?

I believe Alex shall know. Pinging @CAMOBAP

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Dec 7, 2022

@maxirmx unfortunatelly I didn't finish this PR(

BTW how do you think, is it makes sense to switch to some package manager (like vcpgk or conan) to simplify build scripts?

@maxirmx
Copy link
Member

maxirmx commented Dec 10, 2022

TW how do you think, is it makes sense to switch to some package manager (like vcpgk or conan) to simplify build scripts?

vcpkg actually rebuilds everything on each run. It works if it is possible to set up a cache and reuse the artifacts.
However, this project uses several 32-bit workflows and GHA cache action does not exist for 32-bit workflows because cache action depends on Node.js and there is no 32-bit binary ditribution for it.
Also I do not think there is vcpkg version that reliably works with MSys
So, vcpkg can be applied only partially and won't cover the most demanding configurations

I cannot argue about conan, since I do not have any experience with it,

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Dec 11, 2022

@maxirmx thanks for the feedback

I propose to close this PR for now, and maybe return back to it later

@CAMOBAP CAMOBAP closed this Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants