Correct CWD #5

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@jrarmstro

11-sysop.cgi and 11-dragon.cgi boldly that 11-config.pl will be located in the current working directory, which is not always the case:
require "11-config.pl";

This results in errors when the CWD is not the same as the 11-sysop.cgi file location:
Can't locate 11-config.pl in @INC (@INC contains: C:/Strawberry/perl/site/lib C:/Strawberry/perl/vendor/lib C:/Strawberry/perl/lib) at C:/[omitted]/DragonBasher/cgi-bin/11-dragon/11-sysop.cgi line 15.

This solves that issue by adding the parent directory of these files to the @inc.

@jrarmstro jrarmstro closed this Aug 22, 2017

@jrarmstro jrarmstro deleted the jrarmstro:correct-cwd branch Aug 22, 2017

@jrarmstro jrarmstro restored the jrarmstro:correct-cwd branch Aug 22, 2017

@jrarmstro jrarmstro deleted the jrarmstro:correct-cwd branch Aug 22, 2017

@jrarmstro jrarmstro restored the jrarmstro:correct-cwd branch Aug 22, 2017

@jrarmstro jrarmstro reopened this Aug 22, 2017

@jrarmstro jrarmstro closed this Aug 27, 2017

@jrarmstro jrarmstro deleted the jrarmstro:correct-cwd branch Aug 27, 2017

@Quasic

This comment has been minimized.

Show comment Hide comment
@Quasic

Quasic Aug 29, 2017

Owner

Thanks for the PR. I'm not sure if adding to @inc should be done by default, but it should probably be an option, if the cgi system might not set up the CWD as we assume...

Owner

Quasic commented Aug 29, 2017

Thanks for the PR. I'm not sure if adding to @inc should be done by default, but it should probably be an option, if the cgi system might not set up the CWD as we assume...

@jrarmstro

This comment has been minimized.

Show comment Hide comment
@jrarmstro

jrarmstro Aug 29, 2017

I finally managed to make Apache add the directory to @inc, and closed this issue. Definitely worth keeping in mind though

I finally managed to make Apache add the directory to @inc, and closed this issue. Definitely worth keeping in mind though

@jrarmstro

This comment has been minimized.

Show comment Hide comment
@jrarmstro

jrarmstro Aug 29, 2017

From my reading the general consensus seemed to be that you should never assume the CWD will be correct, although we may cut corners in this case for performance reasons

From my reading the general consensus seemed to be that you should never assume the CWD will be correct, although we may cut corners in this case for performance reasons

@Quasic

This comment has been minimized.

Show comment Hide comment
@Quasic

Quasic Aug 30, 2017

Owner

We probably don't need to worry so much about performance in 11-sysop.cgi. If we can detect the problem, there, we could issue a helpful error message, maybe applying your fix, or leave that as an option the user can select to continue setup...

Owner

Quasic commented Aug 30, 2017

We probably don't need to worry so much about performance in 11-sysop.cgi. If we can detect the problem, there, we could issue a helpful error message, maybe applying your fix, or leave that as an option the user can select to continue setup...

@Quasic

This comment has been minimized.

Show comment Hide comment
@Quasic

Quasic Aug 30, 2017

Owner

Joe would prefer we either change CWD or use $cgidir to prefix all .pl scripts, like we do for other paths.

I'm thinking that if we just remove all text after the last slash (backslash may need detecting on windows) we wouldn't need a module to set the variable, but I'd like to see how the module does it...

EDIT: the last slash should also be removed, so we can use it like "$cgidir/config.pl"

Owner

Quasic commented Aug 30, 2017

Joe would prefer we either change CWD or use $cgidir to prefix all .pl scripts, like we do for other paths.

I'm thinking that if we just remove all text after the last slash (backslash may need detecting on windows) we wouldn't need a module to set the variable, but I'd like to see how the module does it...

EDIT: the last slash should also be removed, so we can use it like "$cgidir/config.pl"

@jrarmstro

This comment has been minimized.

Show comment Hide comment
@jrarmstro

jrarmstro Aug 30, 2017

Simply adding the CWD to @inc should cover a lot of cases too, since it seems CWD is not included in @inc by default by some interpreters etc.

(untested)
use lib '.';

You can also do a lot of manipulation based on $0 etc

Simply adding the CWD to @inc should cover a lot of cases too, since it seems CWD is not included in @inc by default by some interpreters etc.

(untested)
use lib '.';

You can also do a lot of manipulation based on $0 etc

@Quasic

This comment has been minimized.

Show comment Hide comment
@Quasic

Quasic Aug 30, 2017

Owner

man lib says the module is kept small, and the path is placed at the first-searched position, so performance shouldn't be a problem for the actual includes, if used after any other modules.

use lib "/unix/path";

as well as direct setting of @inc might be tricky with some systems, but

use lib '.';

or just modifying @inc directly if '.' isn't found in the right position, might work until we have a good way to get a unix path to the parent folder.

Owner

Quasic commented Aug 30, 2017

man lib says the module is kept small, and the path is placed at the first-searched position, so performance shouldn't be a problem for the actual includes, if used after any other modules.

use lib "/unix/path";

as well as direct setting of @inc might be tricky with some systems, but

use lib '.';

or just modifying @inc directly if '.' isn't found in the right position, might work until we have a good way to get a unix path to the parent folder.

@Quasic

This comment has been minimized.

Show comment Hide comment
@Quasic

Quasic Aug 30, 2017

Owner

I just tried a test in one setup, and lib moved '.' from the end to the start of @INC, which I expect would actually improve performance.

Owner

Quasic commented Aug 30, 2017

I just tried a test in one setup, and lib moved '.' from the end to the start of @INC, which I expect would actually improve performance.

Quasic added a commit that referenced this pull request Sep 1, 2017

@Quasic

This comment has been minimized.

Show comment Hide comment
@Quasic

Quasic Sep 1, 2017

Owner

I don't think we need to worry about memleaks with one possible duplication in the list, and no more libraries are loaded after this, anyway, so I left out that part of what lib.pm does. We could just replace the list with the one path, but I didn't want to impose that limit, here, just in case...

Owner

Quasic commented Sep 1, 2017

I don't think we need to worry about memleaks with one possible duplication in the list, and no more libraries are loaded after this, anyway, so I left out that part of what lib.pm does. We could just replace the list with the one path, but I didn't want to impose that limit, here, just in case...

@Quasic

This comment has been minimized.

Show comment Hide comment
@Quasic

Quasic Sep 6, 2017

Owner

I was going to add it to 11-dragon.cgi after it was working in 11-sysop.cgi, but I think I'll move it over, now, and fix another issue, while I'm at it.

Owner

Quasic commented Sep 6, 2017

I was going to add it to 11-dragon.cgi after it was working in 11-sysop.cgi, but I think I'll move it over, now, and fix another issue, while I'm at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment