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

Support for Darwin @xxx placeholders in dll-path #335

Closed
wants to merge 4 commits into from

Conversation

JBouwer
Copy link
Contributor

@JBouwer JBouwer commented Sep 14, 2023

Support for Darwin @xxx placeholders in dll-path.

The Darwin platform supports special @xxx/ placeholders for linking into the run path list of binaries.

See the discussion #332 for background and details, as well as my rationale for choosing the following resolution.

The accompanying pull request resolves the issue as follows:

  1. A new at-placeholder-translate-path-rule rule in src/tools/darwin.jam.

    The rule conforms to the translate-path signature, and is exported to the global namespace with a qualifying "darwin." prefix.

  2. Modify property.translate in src/build/property.jam to allow for multiple translate-path-rules; The first rule to return a successful translation breaks the loop.

  3. Add the darwin.at-placeholder-translate-path-rule from (1) to the list of translate-path-rules - whenever src/tools/darwin.jam is loaded.

    I could not come up with a better way to do this - any alternative suggestions will be welcomed.

Types of changes

What types of changes does your code introduce?

Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Documentation content changes
  • Other (please describe):

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

…placeholders.

* src/tools/darwin.jam
(at-placeholder-translate-path-rule): New <translate-path> rule with global import.
* src/build/property.jam
(property.translate): Rename 'translate-path-rule' to plural, and iterate over each element until successful translation (or not).

* src/tools/features/translate-path-feature.jam
    Document the above change.
* src/build/property.jam
    (add-toolset-translate-path-rule): New rule to append to 'translate-path-rules'.
    (translate): Elevated local variable 'translate-path-rules' to module scope.

* src/tools/darwin.jam
    (at-placeholder-translate-path-rule): Use above to append 'darwin.at-placeholder-translate-path-rule' to 'translate-path-rules'.
@JBouwer
Copy link
Contributor Author

JBouwer commented Sep 14, 2023

Documentation for the above in 205b597, on top of generic documentation update PR #334.

Copy link
Member

@grafikrobot grafikrobot left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Generally the idea is good. Just needs some tweaks to behave better with the multi-toolset/multi-variant build process. One FYI though.. Our goal is to eventually remove the darwin toolset. And instead do all the equivalent handling it does in the clang toolset.

@@ -561,7 +567,7 @@ rule translate-indirect-value ( rulename : context-module )
#
rule translate ( properties * : project-id : project-location : context-module )
{
local translate-path-rule = [ MATCH "^<translate-path>[@](.*)$" : "$(properties)" ] ;
translate-path-rules += [ MATCH "^<translate-path>[@](.*)$" : "$(properties)" ] ;
Copy link
Member

Choose a reason for hiding this comment

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

Having this be global will have the effect of a compounding effect of multiple redundant translate rules being added. The rules could also not be related to the particular targets as they are global and will be run for all target and toolsets, not just the darwin one, if one happens to build with multiple toolsets at once. Hence this needs to be a plain local translate-path-rules = ....

Comment on lines +555 to +560
local translate-path-rules ;
rule add-toolset-translate-path-rule ( tpr )
{
translate-path-rules += $(tpr) ;
}

Copy link
Member

Choose a reason for hiding this comment

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

Adding this globally has some serious side effects (see the comment in translate rule). There's a better way to achieve a similar "global" effect with toolset.add-requirements but without the drawbacks.

Comment on lines +619 to +629
# Iterate through each translate path rule in turn,
# until one returns a successful translation.
for local tpr in $(translate-path-rules)
{
value = [ $(translate-path-rule) $(feature) $(property:G=) : $(properties) : $(project-id) : $(project-location) ] ;
value = [ $(tpr) $(feature) $(property:G=) : $(properties) : $(project-id) : $(project-location) ] ;
if $(value)
{
break ;
}
}
# If no translate path rule intervened, continue with usual translation.
Copy link
Member

Choose a reason for hiding this comment

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

This looks fine.

Comment on lines +483 to +488
# Import the 'at-placeholder-translate-path-rule' into the global namespace
# as 'darwin.at-placeholder-translate-path-rule'.
IMPORT $(__name__) : at-placeholder-translate-path-rule : : darwin.at-placeholder-translate-path-rule ;
# Add the above exported name to the list of translate path rules for property.translate.
property.add-toolset-translate-path-rule darwin.at-placeholder-translate-path-rule ;

Copy link
Member

Choose a reason for hiding this comment

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

This you need to replace with an appropriate call to toolset.add-requirement in the rule init of the darwin toolset. In particular it should add a conditional (on the toolset $(condition)) that evaluates to a <translate-path>@darwin.at-placeholder-translate-path-rule. Search for toolset.add-requirements to see examples of how it should be called.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, scratch all that.. I see now how toolset-add-requirements will not work for this.

@grafikrobot
Copy link
Member

Decided to go a much simpler way to resolve this. See d1c2312

@grafikrobot grafikrobot closed this Dec 3, 2023
@JBouwer
Copy link
Contributor Author

JBouwer commented Dec 3, 2023

Excellent! 👌
Thanks for considering!

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.

2 participants