Skip to content

Inject DLL paths on TestFoundation #3146

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stevapple
Copy link
Contributor

Patch DLL paths to environment on building and running the tests. This eliminates the need to modify Path manually and adds to robustness.

@stevapple
Copy link
Contributor Author

Anyone to review?

@stevapple stevapple mentioned this pull request Mar 16, 2022
@millenomi
Copy link
Contributor

cc @compnerd ?

@compnerd
Copy link
Member

I'm not a fan of the use of ? as a separator, and I would rather that we change that to something more inline with CMake's list operations. More importantly, I think that this needs to be tested on CI to ensure that this doesn't accidentally cause any regressions. Likely best to wait until after 5.7.

@stevapple
Copy link
Contributor Author

If we keep ; as separator we may run into the hell of extra //s, and any additional parse may add an extra //, making it extremely hard to maintain (since Windows CI is not forced yet, I’m really pessimistic about not breaking this some day.

Taking another separator at least simplifies the implementation and adds to stability — and I picked ? from forbidden characters in regular Windows path (I knew there’re \\?\ paths, but I don’t think they’re working & worth being supported).

@stevapple
Copy link
Contributor Author

Since 5.7 branch was cut off, I think it's time to verify if it can fit into the build workflow. There should involve some updates to the build script and may you have a look at it? @compnerd

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