Skip to content

Conversation

@g-arjones
Copy link
Contributor

CMake automatically adds the parent folder of each path in PATH to its search paths, (partially) defeating the purpose of the setting.

@g-arjones g-arjones requested a review from doudou July 15, 2025 18:19
@g-arjones
Copy link
Contributor Author

g-arjones commented Jul 15, 2025

To me, it would also be fine to always install to prefix/autoproj/bin regardless of config.separate_prefixes?. @doudou Thoughts?

@g-arjones
Copy link
Contributor Author

@doudou Ping?

@doudou
Copy link
Member

doudou commented Jul 24, 2025

The shims are global even with separate_prefixes?. Mixing different versions of python or ruby in a single workspace would be very fragile.

separate_prefixes? is meant to isolate the packages, not the primary tooling from the workspace. In any case, this patch would break a lot of stuff, no ? You would at least need to add the new path to Autoproj's internal path, and make sure it ends up where it should in env.sh.

@g-arjones
Copy link
Contributor Author

g-arjones commented Jul 24, 2025

Mixing different versions of python or ruby in a single workspace would be very fragile.

That's not the goal. The issue I'm trying to fix is that cmake will add the parent folder of each folder in PATH to its internal search paths. This means that #{Autoproj.prefix}/install always ends up in cmake's search paths and it will sometimes use or find stuff that it should not when using separate_prefix = true because cmake does a lot of heuristics when it is looking for things. If the shims are in #{Autoproj.prefix}/install/autoproj/bin instead then #{Autoproj.prefix}/install/autoproj will be added (which is fine, because that folder will only have the shims anyway).

in any case, this patch would break a lot of stuff, no ?

I don't see why, and it didn't in my tests but I can put it behind a configuration flag if you prefer.

You would at least need to add the new path to Autoproj's internal path, and make sure it ends up where it should in env.sh.

It's there already (the second line following the last line of the patch)

@g-arjones
Copy link
Contributor Author

@doudou Ping?

(Sorry for insisting, I'm in the process of resyncing our internal mirrors and would like to get these PRs in first)

@doudou
Copy link
Member

doudou commented Aug 2, 2025

@doudou Ping?

(Sorry for insisting, I'm in the process of resyncing our internal mirrors and would like to get these PRs in first)

Apologies on my side as well ... The last two weeks were pretty intense.

Fine then. But why not move it in both cases (separate_prefixes and normal) ? Wouldn't the same issue apply with separate_prefixes = false ?

@doudou
Copy link
Member

doudou commented Aug 2, 2025

I'm also worried about this breaking existing builds ... I'd rather not publish autoproj 3 just for this. So please protect the new behaviour with a configuration flag.

@g-arjones
Copy link
Contributor Author

But why not move it in both cases (separate_prefixes and normal) ?

I'm fine with that.

Wouldn't the same issue apply with separate_prefixes = false ?

It would, but that's kind of expected in that case because all packages share the same prefix anyway.

I will change the patch to have the shims always in a separate prefix, regardless of config.separate_prefixes, and have it behind a flag.

@doudou Could you please have a look at #395 ? That one has been open since 2023.

@g-arjones g-arjones force-pushed the install_ruby_shims_in_separate_prefix branch from 8f351e6 to 3dbf1a2 Compare August 12, 2025 20:42
@g-arjones
Copy link
Contributor Author

@doudou Done!

@g-arjones g-arjones changed the title consider 'config.separate_prefixes?' when installing ruby shims allow installing ruby shims in a separate prefix Aug 12, 2025
@doudou doudou merged commit 0860457 into master Aug 13, 2025
6 checks passed
@doudou doudou deleted the install_ruby_shims_in_separate_prefix branch August 13, 2025 16:49
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.

3 participants