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

scriptfiles path is incorrect #47

Open
Disinterpreter opened this issue Jun 19, 2020 · 9 comments
Open

scriptfiles path is incorrect #47

Disinterpreter opened this issue Jun 19, 2020 · 9 comments
Assignees
Labels
bug Something isn't working core The issue depends from the code of the module.

Comments

@Disinterpreter
Copy link
Member

Describe the bug
There is a problem with scriptfiles. Probably my problem is bad reading of the documentation. But I can't work with scriptfiles.

I tried to put scriptfiles into the amx/scriptfiles, and amx-mygamemode/scriptfiles
I tried to move amx into /resources/amx and put file there.
But all my moves didn't work.

To reproduce
Just try to read data from scriptfiles. Or try to launch something like https://github.com/Open-GTO/Open-GTO

Expected behaviour
It must work fine. The files from script files must be available.

Additional context

amx/amx-deps/src/util.cpp

Lines 207 to 219 in 1cb22d5

// Then check if it exists in the main scriptfiles folder
fs::path scriptfilespath = fs::path("mods/deathmatch/resources/amx/scriptfiles") / filename;
if(exists(scriptfilespath))
{
return scriptfilespath.string();
}
// Otherwise default to amx's resource folder - make sure the folder
// where the file is expected exists
fs::path folder = respath;
folder.remove_filename();
create_directories(folder);
return respath.string();

@Disinterpreter Disinterpreter added the bug Something isn't working label Jun 19, 2020
@Disinterpreter Disinterpreter changed the title scriptfiles path an incorrect scriptfiles path is an incorrect Jun 19, 2020
@Disinterpreter Disinterpreter changed the title scriptfiles path is an incorrect scriptfiles path is incorrect Jun 19, 2020
@colistro123
Copy link
Collaborator

The main problem seems to be that the getScriptFilePath function isn't referenced anywhere except for sqlite reading stuff, so the path is probably never set.

@Disinterpreter Disinterpreter added the core The issue depends from the code of the module. label Jun 19, 2020
@Disinterpreter
Copy link
Member Author

Disinterpreter commented Jun 19, 2020

The main problem seems to be that the getScriptFilePath function isn't referenced anywhere except for sqlite reading stuff, so the path is probably never set.

https://github.com/Disinterpreter/pmta-wrapper/search?q=getScriptFilePath&unscoped_q=getScriptFilePath

I think it can help. It is latest version adamix's AMX.

@qaisjp
Copy link
Contributor

qaisjp commented Jun 19, 2020

You may find it useful to incorporate the fix from 1325abf

@theSpool
Copy link

Ah, it's true, AMX has that SA-MP specific change, I just forgot about it when changing the AMX version. I'll have to put it back in.

@theSpool theSpool self-assigned this Jun 21, 2020
@theSpool
Copy link

OK, there's a small inconsistency with SA-MP in the previous AMX. SA-MP uses a single path for the scriptfiles of every AMX script, and, for security reasons (to avoid malicious compiled scripts touching the rest of the file system), the natives aren't allowed to go outside this folder. This module's implementation looks on both for an existing file before defaulting to the script's file.

Although at first glance SA-MP's approach might seem problematic, it's been the standard an some scripts might rely on it. A script might create a file or database and expect another script to use its contents. With this resources implementation, this wouldn't work.

I looked into the current file natives implementation and I noticed it uses a completename function which looks for the env var AMXFILE_VAR (which currently is defined as "AMXFILE"), which can be specified by the user. I'd suggest to use setenv while initializing the module with the overwrite value set to false so users can set it to whatever they want. This would implement and extend (rather than change) SA-MP's behaviour.

Any comments?

@colistro123
Copy link
Collaborator

colistro123 commented Jun 23, 2020

Thanks @theSpool, resolved in 422b841#diff-f5ac81a5af817525061fa6eafe4ca6eaR140

If you have any better solutions, such as this, feel free to add them I just noticed that, my bad:

I'd suggest to use setenv while initializing the module with the overwrite value set to false so users can set it to whatever they want. This would implement and extend (rather than change) SA-MP's behaviour.

@Disinterpreter
Copy link
Member Author

And where the scriptfiles folder must be now?

@qaisjp
Copy link
Contributor

qaisjp commented Jun 23, 2020

It's unfortunately still hardcoded to resources/amx/scriptfiles. So if your amx resource is in a different folder (likely), it won't work.

@Disinterpreter
Copy link
Member Author

It's unfortunately still hardcoded to resources/amx/scriptfiles. So if your amx resource is in a different folder (likely), it won't work.

I don't know what I do incorrect. I build from latest commit. I put scriptfiles. And got an error. (I trying to launch OpenGTO)

@colistro123 colistro123 reopened this Jun 23, 2020
@AGraber AGraber assigned AGraber and unassigned theSpool Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core The issue depends from the code of the module.
Projects
None yet
Development

No branches or pull requests

5 participants