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

Additional Flags for CustomFileBuildStepData #322

Merged
merged 3 commits into from Feb 2, 2024

Conversation

Sharlock93
Copy link
Contributor

This mainly adds two flags, ResolveExecutable and DependOnExecutable.

First one is basically to skip prefixing the Executable string with the project path. This allows running of programs that are in the environment but not in the project.

My use case was running a shader compiler while not adding the executable to the project.

The second flag is to skip adding the executable to the list of AdditionalInputs, this prevents
a warning when the executable is not found in the project.

The first flag has a workaround by overriding MakeRelativePaths but the second flag does not have any workaround I could find.

I'm not extensively familiar with all potential problems that might come up with these flags so any feedback on it would be appreciated.

Sharlock93 added 2 commits December 28, 2023 13:37
This adds two new flags
- ResolveExecutable: whether or not to pass Executable through resolver
- DependOnExecutable: whether or not to add Executable to AdditionalInputs
@@ -2775,7 +2786,10 @@ internal void Resolve(Resolver resolver)
{
customFileBuildStep.Resolve(resolver);
Util.ResolvePath(Project.SourceRootPath, ref customFileBuildStep.KeyInput);
Util.ResolvePath(Project.SourceRootPath, ref customFileBuildStep.Executable);
if(customFileBuildStep.ResolveExecutable)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why this is problematic for your to resolve the sharpmake variables.
if you don't have any variable of the format [project.XXXX] this won't do anything.

I'm not totally against this but I don't understand your use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, thanks for getting back to me!
Maybe I have configured something wrong but when that resolve happens, the project path gets appended to the executable name even if there are no [project.XXXX] variables to resolve. I have attached a screenshot to better demonstrate this, glslang is an executable in my Path and not inside the project tree.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hello,
Sharpmake assumes that the executable is in the SourceRootPath folder since you didn't specified an absolute path. In general however, from a build system point of view, relying on PATH to find your compiler is not really a recommended approach.

There is several reasons to this:

  • Build will fail if your compiler is missing from path
  • You will have build reproducibility issues(two machine could have different version of the compiler installed)

In general at ubisoft for these kind of things we generally use nugets for our various compilers and sdks(vs2022, console sdks etc). In some simple case with only a single .exe we sometimes publish the .exe in our source control. By doing these we insure build reproducibility. You should never rely on what is installed on a pc and especially not rely on what is in the PATH variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the explanation. Those are all valid points when it comes to real production, in my case its my personal codebase and when I use it anywhere I know about the limitations. This is mainly because I don't want to check in glslang because I don't want to waste space on my limited hosted server and git is really bad with binaries.

I'm sure you could find use cases for it across a team in specific situations but I'd imagine those are rare. If you think this sort of goes against the design of sharpmake then we can close the PR however if you think the base idea is valid but needs some changes I'd appreciate any feedback :)

Thank you for taking the time reviewing this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I suggest you to do is if you know the full path to glslang, specify it in sharpmake. I think this will fix your problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh in terms of workarounds or fixing the problem, it also works to just have a custom class for customfildbuildstep. However both cases add a dependency to AdditionalInputs field hence why I have the other flag in there too.

- Consolidated ResolveExecutable and DepeonOnExecutable into one new flag
called UseExecutableFromSystemPath
@jspelletier jspelletier merged commit 0f38fe2 into ubisoft:main Feb 2, 2024
49 checks passed
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.

None yet

2 participants