Skip to content

[vcpkg] Add MAKE_OPTIONS and MAKE_INSTALL_OPTIONS#9569

Closed
SeekingMeaning wants to merge 11 commits intomicrosoft:masterfrom
SeekingMeaning:make-opts
Closed

[vcpkg] Add MAKE_OPTIONS and MAKE_INSTALL_OPTIONS#9569
SeekingMeaning wants to merge 11 commits intomicrosoft:masterfrom
SeekingMeaning:make-opts

Conversation

@SeekingMeaning
Copy link
Contributor

@SeekingMeaning SeekingMeaning commented Jan 7, 2020

This pull request allows for adding options for make and make install. This is especially useful for projects that don't have a configure file and only have a Makefile. Example usage:

vcpkg_configure_make(
    SOURCE_PATH ${SOURCE_PATH}
    SKIP_CONFIGURE
)

vcpkg_install_make(
    MAKE_INSTALL_OPTIONS_DEBUG
        "PREFIX=${CURRENT_PACKAGES_DIR}/debug"
        "BINDIR=${CURRENT_PACKAGES_DIR}/debug/tools/${PORT}"
    MAKE_INSTALL_OPTIONS_RELEASE
        "PREFIX=${CURRENT_PACKAGES_DIR}"
        "BINDIR=${CURRENT_PACKAGES_DIR}/tools/${PORT}"
)

@JackBoosY JackBoosY self-requested a review January 7, 2020 06:44
@JackBoosY JackBoosY self-assigned this Jan 7, 2020
@JackBoosY
Copy link
Contributor

When I developed this set of functions, I didn't consider the scenario where I need to pass parameters when executing make.
But for the installation path, it is already set by default[1]. You can also disable the default setting[2].

[1].

set(_csc_OPTIONS_RELEASE ${_csc_OPTIONS_RELEASE}

[2]. https://github.com/microsoft/vcpkg/blob/c0d22c88ea7638d1b74339f9e9adfd37b0f525ed/docs/maintainers/vcpkg_configure_make.md#disable_auto_dst

@SeekingMeaning
Copy link
Contributor Author

Hm... This would still require configure (or configure.in if using Autoconfig), right?

@JackBoosY
Copy link
Contributor

@SeekingMeaning I know what you mean. And my suggestion is to rename INSTALL_OPTIONS to MAKE_INSTALL_OPTIONS to avoid misunderstandings as settings during configuration.

@JackBoosY
Copy link
Contributor

Could you please add some example to check it?

@SeekingMeaning
Copy link
Contributor Author

Like a port? I have a local version of #9558 that uses this, is that okay?

@JackBoosY
Copy link
Contributor

Yes please.

@SeekingMeaning
Copy link
Contributor Author

Oh whoops, I forgot to update for the INSTALL_OPTIONS name change.

@SeekingMeaning SeekingMeaning changed the title [vcpkg] Add MAKE_OPTIONS and INSTALL_OPTIONS [vcpkg] Add MAKE_OPTIONS and MAKE_INSTALL_OPTIONS Jan 7, 2020
@JackBoosY
Copy link
Contributor

/azp run

@SeekingMeaning
Copy link
Contributor Author

Quick question: should the port be removed before merge?

@JackBoosY
Copy link
Contributor

@SeekingMeaning Hmm... I think you can combine two PRs into one and close #9558.

@SeekingMeaning
Copy link
Contributor Author

SeekingMeaning commented Jan 9, 2020

Another question: I have this working with LuaJIT (which already exists as a port); should I wait to open a new pull request or add the changes to this one?

@JackBoosY
Copy link
Contributor

If your changes are related to this PR, please submit to this PR.

Thanks.

@SeekingMeaning
Copy link
Contributor Author

Anything else?

@JackBoosY
Copy link
Contributor

@SeekingMeaning No, we are waiting for fix osx PR test. See #9600.

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Unfortunately, CMake is much more performant when using a mutating style (via list(APPEND)) instead of a "constant single assigment" style like you're using here (XYZ_BASE, then set(XYZ ${XYZ_BASE} wqv)).

Please prefer the agglutinating, additive style using list(APPEND) instead.

@dan-shaw
Copy link
Contributor

Somehow we lost track of this PR. @SeekingMeaning can you fix the conflict?

@SeekingMeaning
Copy link
Contributor Author

Sure, I'll have to open a new pull request though since the original fork was deleted

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