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

Check for settings.json in current directory as well #3436

Merged
merged 4 commits into from
Jun 4, 2021

Conversation

rajat2004
Copy link
Contributor

@rajat2004 rajat2004 commented Mar 3, 2021

Fixes: #3428

About

Adds a check for settings.json in the directory from where the binary was launched
For e.g. in Linux, if invoking Blocks.sh from inside the LinuxNoEditor folder, the path searched would be .../LinuxNoEditor/settings.json. If started from a level above using ./LinuxNoEditor/Blocks.sh, it would be .../settings.json.

Currently, it's an additional check rather than replacing the executable location since that'll be a behavioural change, can be removed if required.

Also, this again brings up the usefulness of allowing download on the compiled binaries from the Azure Pipelines CI. It'll allow much easier testing of PRs on different platforms since packaging locally takes time, and cross-platform compilation has problems, cannot compile for Win from Linux, and for Linux requires separate toolchain install on Windows.

If its preferred to have even more checks such as inside the project location (LinuxNoEditor/Blocks/settings.json), or maybe keep it fixed as the root directory instead of the current variable one, please do comment.

How Has This Been Tested?

The earlier method didn't work and hence has been removed completely. Compiled a Linux binary and tested the path being searched, here's the binary

  • /home/rajat/AirSim/Unreal/Environments/Blocks/Packaged/Development/LinuxNoEditor/settings.json when inside the LinuxNoEditor folder
  • /home/rajat/AirSim/Unreal/Environments/Blocks/Packaged/Development/settings.json when outside it

Windows testing has been done by @ahmed-elsaharti, thanks a lot!

Screenshots (if appropriate):

@ahmed-elsaharti
Copy link
Contributor

Hey @rajat2004, ran the sanity test on Windows (x64) and it seems to work as expected.
I did have to replace getcwd with _getcwd and unistd.h with direct.h though.

Will try to test the fork on Windows soon

@rajat2004
Copy link
Contributor Author

@ahmed-elsaharti Thanks a lot for testing! I actually built the binary and tried it out, didn't work, so trying a different way right now, will push a commit soon.

Side note, I wrote a small helper script to package the project, for Linux, otherwise requires the Editor to be open all the time, which is quite heavy separately. Am having some problem with the entire UE4 and Editor getting compiled entirely, every single time!. Makes packaging a 3-4 hr process on my system :(.

@ahmed-elsaharti
Copy link
Contributor

ahmed-elsaharti commented Mar 4, 2021

@rajat2004 My pleasure 👍 Also, I really like the idea of relying on ProjectDir() here since its not an OS library dependant function.
Btw, if you ever need Windows testing with any PR just @ me and I'd be happy to test :D

Also, I tested 6c81440 (0cec4f6 was committed as I was writing this 😂) and on the editor it looked for settings.json in the same directory as the sln file. With a compiled binary it did not do the same (tbh I can't figure out where Utils::log writes to when running a binary so I cant track down the path it actually looks at). I'll try 0cec4f6 to see if switching over to an absolute path fixes things.

And regarding the Linux packaging issue, it's things like this that make me change my mind whenever I decide to make the move to Linux 😂

@rajat2004
Copy link
Contributor Author

This is somewhat working, the current paths being searched are -

  1. /home/rajat/AirSim/Unreal/Environments/Blocks/Packaged/Development/LinuxNoEditor/Blocks/Binaries/Linux/settings.json
    The too-deep location

  2. /home/rajat/AirSim/Unreal/Environments/Blocks/Packaged/Development/LinuxNoEditor/Blocks/settings.json
    Somewhat better, its still one folder deeper than the top-level script directory where Blocks.sh is located

  3. /home/rajat/Documents/AirSim/settings.json

Might be that hardcoding the path to just go another level up is required, but will try to keep looking for another way

@ahmed-elsaharti
Copy link
Contributor

ahmed-elsaharti commented Mar 4, 2021

On Windows it looks for:

  1. C:\GitRepos\Bin\new\WindowsNoEditor\Blocks\Binaries\Wind64\settings.json
  2. C:/GitRepos/Bin/new/WindowsNoEditor/Blocks/settings.json
  3. C:\Users\ahmed\OneDrive\Documents\AirSim\settings.json

Also, I found FPaths::ConvertRelativePathToFull that could be used instead of creating a new getAbsProjectDir method if I'm not mistaken.

Edit: maybe have both the 'one folder deeper' and one where it goes another level up? Since the former would be useful if the user wants to place their settings.json in the same folder as their Environment.sln (could be helpful) and the latter would do what you're originally trying to do

@ahmed-elsaharti
Copy link
Contributor

ahmed-elsaharti commented Mar 5, 2021

@rajat2004 FPaths::LaunchDir() should do the trick on Windows (not entirely sure if it does the same on Linux but it theoretically should).
image
(I threw in a UAirBlueprintLib::LogMessage(settingsFilepath, TEXT(""), LogDebugLevel::Informational); in ASimHUD::readSettingsTextFromFile just to be able to see what directories its searching in)

1st one is FString(msr::airlib::Settings::getExecutableFullPath("settings.json").c_str()) aka the too deep location
2nd one is FPaths::Combine(FPaths::LaunchDir(), FString("settings.json")) which ends up being the location of the executable

@rajat2004
Copy link
Contributor Author

Also, I found FPaths::ConvertRelativePathToFull that could be used instead of creating a new getAbsProjectDir method if I'm not mistaken.

Yeah, there are different methods available, ConvertRelativePathToFull also works, just tested it out. Reg the getAbsProjectDir method, I think it'll be cleaner to have a separate method since otherwise it would require squishing normally 3 lines of code into 1, which is just much more difficult to read. Once it works, the name can be improved

Edit: maybe have both the 'one folder deeper' and one where it goes another level up? Since the former would be useful if the user wants to place their settings.json in the same folder as their Environment.sln (could be helpful) and the latter would do what you're originally trying to do

Sure, if that's usefull I'm okay with it. IMO using the --settings arg is the cleanest way to avoid all this, but whatever's comfortable

As to the Utils::log, it prints it out to stdout or stderr depending on the LogLevel, on Linux it appears on the terminal, so much easier to view and refer to the output later on as well, especially if there's a lot of logging to be done :). I think we need another logging method that does both 😉

FPaths::LaunchDir() should do the trick on Windows (not entirely sure if it does the same on Linux but it theoretically should).

Yup, it works. I also tried RootDir() which also worked. I think LaunchDir might refer to from where the script was launched, which might be the better way. Currently testing LaunchDir + ConvertRelativePathToFull only

@rajat2004 rajat2004 force-pushed the settings-location branch from 0cec4f6 to ed30bca Compare March 5, 2021 10:24
@rajat2004 rajat2004 marked this pull request as ready for review March 5, 2021 10:38
@ahmed-elsaharti
Copy link
Contributor

Sure, if that's usefull I'm okay with it. IMO using the --settings arg is the cleanest way to avoid all this, but whatever's comfortable

Valid point. But on the Windows side users usually (from what I've seen) launch binaries the easy way by double-clicking rather than cmd-ing. This PR would be tackling the settings issue when doing that though so it should be all good 👍

As to the Utils::log, it prints it out to stdout or stderr depending on the LogLevel, on Linux it appears on the terminal, so much easier to view and refer to the output later on as well, especially if there's a lot of logging to be done :). I think we need another logging method that does both 😉

Yup, 2 hours into the searching process I finally found that tiny output window on VS2019 😂. But I agree, the logging method could probably be revamped 😎

Yup, it works. I also tried RootDir() which also worked. I think LaunchDir might refer to from where the script was launched, which might be the better way. Currently testing LaunchDir + ConvertRelativePathToFull only

I'll test the latest commit out on Windows and will report back 👍

@ahmed-elsaharti
Copy link
Contributor

ahmed-elsaharti commented Mar 5, 2021

ed30bca tested on Windows and works perfectly.
image
Corresponding binary: BlocksWin

@zimmy87 zimmy87 merged commit 84e9453 into microsoft:master Jun 4, 2021
@rajat2004 rajat2004 deleted the settings-location branch June 4, 2021 18:10
@rajat2004
Copy link
Contributor Author

@zimmy87 Thanks a lot for fixing the formatting and merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

settings.json is searched for in an unexpected location. A fix?
4 participants