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

Show error messages from broken includes #2264

Merged

Conversation

richard-sim
Copy link
Contributor

@richard-sim richard-sim commented Sep 27, 2024

What does this PR do?

Displays the error message returned from loadfile(), which includes the file and line information (necessary for nested includes!), as well as the specific error that was encountered.

How does this PR change Premake's behavior?

Rather than the generic "Cannot find..." message, if there's error information available it's now displayed:

Before:

Error: ** Error: Cannot find either foobar/L0a or foobar/L0a.lua or foobar/L0a/premake5.lua or foobar/L0a/premake4.lua

After:

Error: ** Error: Cannot load foobar/L0a: D:/dev/build-sys-eval/foobar/src/foobar/L0a/premake5.lua:7: '(' expected near '['

Anything else we should know?

Nope, it's a simple one but really, REALLY helps!

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits
  • Align documentation to your changes

- Display the error message returned from loadfile(), which includes the file and line information (necessary for nested includes!), as well as the specific error that was encountered.
src/base/_foundation.lua Outdated Show resolved Hide resolved
Integrated PR feedback:
- It can be assumed that err is not nil if compiled_chunk is nil (confirmed with the lua docs)
- Check if the file was located before trying to load it, so we can display a descriptive error message with the alternate names that were searched
- Improved the error messages by addubg the file/line information where the include originated, similar to the output of a compiler. The `res` is not output to the error message as in every case it was already part of the error message that was returned and that resulted in very long, repetitive errors
Depending on the specific error, sometimes code will load but be unable to execute, throwing an error. This now catches (pcall, "protected call") it and displays an appropriate error message, including the file that was included and where the include originated from.
@samsinsane samsinsane merged commit b7f4e1b into premake:master Sep 28, 2024
12 checks passed
@Jarod42
Copy link
Contributor

Jarod42 commented Sep 29, 2024

@samsinsane:
Now we got:

Error: ** Error: [string "src/_premake_main.lua"](70): Error executing '$/vstudio/_preload.lua: ** Error: [string "vstudio/_preload.lua"](6): Cannot find neither vs2005.lua nor vs2005.lua.lua nor vs2005.lua/premake5.lua nor vs2005.lua/premake4.lua

Embed scripts seems problematic (when not called from repository)

 		local res = os.locate(fname, with_ext, p5, p4)
-		res = res or fname

Not sure if issue was in os.locate and res = res or fname was a workaround, but it is currently needed for embed scripts.

@samsinsane
Copy link
Member

@Jarod42 I'm having trouble reproducing this error, do you have a minimal repro script?

@Jarod42
Copy link
Contributor

Jarod42 commented Sep 29, 2024

I got it for any script on my side :-/ (outside of premake-core repo)
on premake-ninja action: even premake --version (without premake5.lua)
https://github.com/jimon/premake-ninja/actions/runs/11075499881/job/30799398854

Try to move binary outside of repository, so os.locate cannot find file from repo and requires to find the embed script.

@samsinsane
Copy link
Member

@Jarod42 sorry, I hadn't rerun the embed action. 🤦 Just finalising the fix and I'll submit the PR for it soon.

@richard-sim
Copy link
Contributor Author

richard-sim commented Sep 29, 2024

@Jarod42 @samsinsane : hrm, I'm not sure how I haven't hit that in my testing & usage?! As you can see from my earlier comment on this PR, I did test with internal scripts (** Error: D:\dev\build-sys-eval\foobar\src\tools\premake\badcode.lua(59): include($/self-test/test_declare.lua) execution error: [string "self-test/test_declare.lua"]:6: attempt to index a nil value (local 'm')), and have been happily using this change for a few days now.

I know what the problem is though - when I was working on this change, I noticed that the _preload.lua files are NOT listed as internal scripts "for some reason", so from @Jarod42's error Error executing '$/vstudio/_preload.lua, that would indeed not work with os.locate(). So yes, I'm almost certain that the res = res or fname was a hack. Why though...? oh nope, that was the manifest files.

@Jarod42 : I'm not sure how you're hitting this and I'm not, but could you perhaps make a PR for a unit test that repos it, so I can work on a proper fix? I'm wondering if this is a Linux-only issue, as your CI was running on Linux, but I'm testing on Windows.

@richard-sim
Copy link
Contributor Author

@Jarod42 : wait... how... wha... ok. Your CI is building premake with -DPREMAKE_NO_BUILTIN_SCRIPTS, so nothing's being embedded, and you'll never have a _SCRIPT_DIR beginning with $/ (I assume).

const buildin_mapping* premake_find_embedded_script(const char* filename)
{
#if !defined(PREMAKE_NO_BUILTIN_SCRIPTS)
	int i;
	for (i = 0; builtin_scripts[i].name != NULL; ++i) {
		if (strcmp(builtin_scripts[i].name, filename) == 0) {
			return builtin_scripts + i;
		}
	}
#endif
	return NULL;
}

I'm trying to understand the differences between preloadModules() and moduleLoader(name) in _premake_main.lua, as the later uses several path formats that the former does not (and it's the former that's failing), which is a little sus.

@samsinsane
Copy link
Member

@richard-sim I've merged the fix. premake.findProjectScript relied on an extension to loadfile to load embedded files:

/* If the currently running script was embedded, try to load this file
* as it if were embedded too. */
if (z != OKAY) {
lua_getglobal(L, "_SCRIPT_DIR");
script_dir = lua_tostring(L, -1);
if (script_dir && script_dir[0] == '$') {
/* Call `path.getabsolute(filename, _SCRIPT_DIR)` to resolve any
* "../" sequences in the filename */
lua_pushcfunction(L, path_getabsolute);
lua_pushstring(L, filename);
lua_pushvalue(L, -3);
lua_call(L, 2, 1);
test_name = lua_tostring(L, -1);
/* if successful, filename and chunk will be on top of stack */
z = premake_load_embedded_script(L, test_name + 2); /* Skip over leading "$/" */
/* remove test_name */
lua_remove(L, -3);
}
/* remove _SCRIPT_DIR */
lua_remove(L, bottom + env);
}

The change I suggested in this PR prevents findProjectScript from calling loadfile on embedded scripts since they won't be found on disk. I just made findProjectScript also look for embedded scripts using the same logic as loadfile, which makes things a bit more obvious instead of magic.

@richard-sim
Copy link
Contributor Author

@samsinsane : ah, thanks for clearing that up - appreciated! It might be good to add a comment to both that loadfile extension and findProjectScript referring to eachother, as it may save some confusion in the future.

I'm still confused how @Jarod42 is hitting this though, as he's building premake with no embedded scripts (-DPREMAKE_NO_BUILTIN_SCRIPTS), so it seems that change shouldn't affect him? Or even by that logic, he shouldn't have run into any issues in the first place.

@samsinsane
Copy link
Member

That's just how the Premake Bootstrap.mak works, here's the steps for Linux:

premake-core/Bootstrap.mak

Lines 109 to 113 in 910c1c5

mkdir -p build/bootstrap
$(CC) -o build/bootstrap/premake_bootstrap -DPREMAKE_NO_BUILTIN_SCRIPTS -DLUA_USE_POSIX -DLUA_USE_DLOPEN -I"$(LUA_DIR)" -I"$(LUASHIM_DIR)" $(SRC) -lm -ldl -lrt -luuid
./build/bootstrap/premake_bootstrap embed
./build/bootstrap/premake_bootstrap --to=build/bootstrap gmake2
$(MAKE) -C build/bootstrap -j`getconf _NPROCESSORS_ONLN` config=$(CONFIG)

Once it builds the bootstrap binary, it builds the proper Premake5 binary with everything enabled.

@Jarod42
Copy link
Contributor

Jarod42 commented Sep 30, 2024

if this is a Linux-only issue, as your CI was running on Linux, but I'm testing on Windows.

No, it happens on all platforms Ubuntu/Windows/MacOS

but could you perhaps make a PR for a unit test that repos it

In fact, my whole premake integrating testing repo fails, and also premake modules (as premake-ninja)

I'm still confused how @Jarod42 is hitting this though

I use the Bootstrap.mak, which seems to be the official way to release version.
I suspected you didn't hit the issue by working from/inside the premake tree (so files can be found directly).

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