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

Added "native" kwarg to add_XXX_args. Closes #3669. #3921

Closed
wants to merge 1 commit into from

Conversation

jpakkane
Copy link
Member

No description provided.

## Projects args can be set separately for cross and native builds (potentially breaking change)

It has been a longstanding bug (or let's call it a "delayed bug fix")
that if yodo this:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Typo yodo.

@jpakkane jpakkane force-pushed the nativeargs branch 2 times, most recently from a52bd24 to e515aab Compare July 22, 2018 21:18
keyword `native` has been added to all functions that add arguments,
namely `add_global_arguments`, `add_global_link_arguments`,
`add_project_arguments` and `add_project_link_arguments` that behaves
like the following:
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph does not mention that from now on the flag will only be used when the cross compiler is invoked.

#ifdef ARG_CROSS
#error "Global is cross but arg_native is set."
#endif
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to add indentation here so that the if/else/endif nesting is clearer.

- `native` is a boolean specifying whether the arguments should be
applied to the native or cross compilation. If omitted, the flags
are added to native targets if compiling natively and cross targets
if cross compiling. Available since 0.48.0
Copy link
Member

Choose a reason for hiding this comment

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

This para is a bit confusing. I would reword it as:

- `native` when set to `true`, the arguments will be added only for 
   native compilations when cross-compiling. If omitted or `false`, 
   the arguments are added to native targets if compiling natively
   and cross targets if cross compiling. Available since 0.48.0

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not the same wording. If set to false then they won't be added to native targets ever. The table goes (or should go at least) like this:

true => only to native builds
false => only to cross builds
omitted => native builds when native compiling, cross builds _only_ when cross building

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it's better to put it in a table form? Should be easier to parse than a paragraph.

## Added only to native compilations, not used in cross compilations.
add_project_arguments(..., native : true)

## Added only to corss compilations, not used in native compilations.
Copy link
Member

Choose a reason for hiding this comment

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

typo corss

@Ericson2314
Copy link
Member

Ericson2314 commented Aug 7, 2018

Per #3972 and #3969, what you actually want is not native vs cross, but build vs host. I.e. some flags are for code-gen tools, some flags are for the final installed artifact, and rarely are flags for both. Crucially: this "rarely" is true whether or not we are cross-compiling.

You could instead have have:

  • no extra arg continue to apply to both for 100% backwards compat
  • target: "build": Apply only to code-gen stuff, i.e. builds that stay native even when cross compiling
  • target: "host": Apply only to installable stuff, i.e. builds that become cross when cross compiling

target: "host" matches the no-arg default this PR currently has, target: "build" has the opposite effect.

@jpakkane
Copy link
Member Author

Should work now.

@jpakkane
Copy link
Member Author

We have native in all other functions like this, for consistency we should use the same here. Having the same thing called different things in different places is bad UX.

@jpakkane
Copy link
Member Author

Typos fixed and merged manually so closing.

@jpakkane jpakkane closed this Aug 22, 2018
@jpakkane jpakkane deleted the nativeargs branch August 22, 2018 20:23
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.

4 participants