Skip to content

Fix bugs in resolve_path() #1238

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

Merged
merged 3 commits into from
Mar 11, 2025
Merged

Fix bugs in resolve_path() #1238

merged 3 commits into from
Mar 11, 2025

Conversation

xexyl
Copy link
Contributor

@xexyl xexyl commented Mar 11, 2025

A couple bugs in the resolve_path() function ended up requiring an extra
check in find_utils() per util in order to get it to find the utils even
outside the directory: even if one had ./ or / in front of the path
it could end up resolving a path that is not a regular executable file
(when it did not have a / in the path this did not happen). Another
bug was fixed where if $PATH is NULL or empty and the path is not a
regular executable file it still would be a strdup()d copy of the
original path.

The above solved a problem where we could get the tools to be found (if
they exist) if we also did a check for is_file() and is_exec() but
with the above fixes we only have to check for != NULL.

The find_utils() functions now takes a corresponding bool * for each
tool to find. If the boolean is not NULL then we search for that tool. I
it is NULL and the char ** is not NULL then we set the *tool to
NULL. This way the caller can, if they wish and they are careful, decide
whether or not to find all the tools. For example txzchk only looks
for tar if test mode is not enabled. And although as a safety check
before free()ing the pointer we do check that test mode was not used we
do not technically need it as long as we pass in &tar (although we do
give NULL so we do need to check it).

xexyl added 3 commits March 11, 2025 06:53
Due to the fact we have to have (for our tools) ./ ahead of other paths
(including the tool names themselves) and also allow for a user to specify a
path (even a bogus one, which if they do we will look for a proper path) and
also search for installed paths, a more careful check has to be done in the
find_utils() functions as otherwise it does not work when out of the repo
directory.

Sync copyright fixes from jparse.
Force set nul_warning to false in mkiocccentry(1) and disable checking of
it in chkentry(1). Post IOCCC28 it will be removed from struct info, the
.info.json files and chkentry(1) that checks it.

Rather than directly put in true in the writing of the .info.json file, force
set first_rule_is_all to true in mkiocccentry(1) and reference that
boolean in the writing of the .info.json file.
A couple bugs in the resolve_path() function ended up requiring an extra
check in find_utils() per util in order to get it to find the utils even
outside the directory: even if one had ./ or / in front of the path
it could end up resolving a path that is not a regular executable file
(when it did not have a / in the path this did not happen). Another
bug was fixed where if $PATH is NULL or empty and the path is not a
regular executable file it still would be a strdup()d copy of the
original path.

The above solved a problem where we could get the tools to be found (if
they exist) if we also did a check for is_file() and is_exec() but
with the above fixes we only have to check for != NULL.

The find_utils() functions now takes a corresponding bool * for each
tool to find. If the boolean is not NULL then we search for that tool. I
it is NULL and the char ** is not NULL then we set the *tool to
NULL. This way the caller can, if they wish and they are careful, decide
whether or not to find all the tools. For example txzchk only looks
for tar if test mode is not enabled.  And although as a safety check
before free()ing the pointer we do check that test mode was not used we
do not technically need it as long as we pass in &tar (although we do
give NULL so we do need to check it).
@xexyl
Copy link
Contributor Author

xexyl commented Mar 11, 2025

Now before you make a new release: should the versions be updated? You could test it on your end (tests of version checks worked on my end). If they work as expected then we could update the guidelines and FAQ to say that if the tools are installed they can use the tools there shouldn't be any problems as long as it's in their path (or in the repo directory if they wish).

I leave this up to you. I know I do have to update the guidelines and maybe FAQ about the warnings recently disabled but I won't get to that today - have other things I need to do. I can do that tomorrow.

Copy link
Contributor

@lcn2 lcn2 left a comment

Choose a reason for hiding this comment

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

👍

@lcn2 lcn2 merged commit 97966d1 into ioccc-src:master Mar 11, 2025
3 checks passed
@lcn2
Copy link
Contributor

lcn2 commented Mar 11, 2025

Now before you make a new release: should the versions be updated? You could test it on your end (tests of version checks worked on my end). If they work as expected then we could update the guidelines and FAQ to say that if the tools are installed they can use the tools there shouldn't be any problems as long as it's in their path (or in the repo directory if they wish).

I leave this up to you. I know I do have to update the guidelines and maybe FAQ about the warnings recently disabled but I won't get to that today - have other things I need to do. I can do that tomorrow.

QUESTION

Which versions are you suggesting be updated?

BTW: We did fix the repo version in CHANGES.md and the related repo version in soup/version.h.

@xexyl
Copy link
Contributor Author

xexyl commented Mar 11, 2025

Now before you make a new release: should the versions be updated? You could test it on your end (tests of version checks worked on my end). If they work as expected then we could update the guidelines and FAQ to say that if the tools are installed they can use the tools there shouldn't be any problems as long as it's in their path (or in the repo directory if they wish).
I leave this up to you. I know I do have to update the guidelines and maybe FAQ about the warnings recently disabled but I won't get to that today - have other things I need to do. I can do that tomorrow.

QUESTION

Which versions are you suggesting be updated?

BTW: We did fix the repo version in CHANGES.md and the related repo version in soup/version.h.

Well: mkiocccentry and chkentry: since those tools no longer check for certain things AND since mkiocccentry now searches under $PATH.

If we do that we could also do txzchk even though there were no functionality changes (well not really - just minor things to fit into the new interface of find_utils() for example). That is not really necessary though.

@lcn2
Copy link
Contributor

lcn2 commented Mar 11, 2025

Now before you make a new release: should the versions be updated? You could test it on your end (tests of version checks worked on my end). If they work as expected then we could update the guidelines and FAQ to say that if the tools are installed they can use the tools there shouldn't be any problems as long as it's in their path (or in the repo directory if they wish).

I leave this up to you. I know I do have to update the guidelines and maybe FAQ about the warnings recently disabled but I won't get to that today - have other things I need to do. I can do that tomorrow.

QUESTION

Which versions are you suggesting be updated?

BTW: We did fix the repo version in CHANGES.md and the related repo version in soup/version.h.

Well: mkiocccentry and chkentry: since those tools no longer check for certain things AND since mkiocccentry now searches under $PATH.

If we do that we could also do txzchk even though there were no functionality changes (well not really - just minor things to fit into the new interface of find_utils() for example). That is not really necessary though.

Please make those version changes that you deem appropriate.

However, please note the repo version changes to CHANGES.md(and the related change to soup/version.h). We have only one pending formal release of this repo.

Also, @xexyl, you might notice that PR #1239 failed.

@xexyl
Copy link
Contributor Author

xexyl commented Mar 11, 2025

Now before you make a new release: should the versions be updated? You could test it on your end (tests of version checks worked on my end). If they work as expected then we could update the guidelines and FAQ to say that if the tools are installed they can use the tools there shouldn't be any problems as long as it's in their path (or in the repo directory if they wish).

I leave this up to you. I know I do have to update the guidelines and maybe FAQ about the warnings recently disabled but I won't get to that today - have other things I need to do. I can do that tomorrow.

QUESTION

Which versions are you suggesting be updated?

BTW: We did fix the repo version in CHANGES.md and the related repo version in soup/version.h.

Well: mkiocccentry and chkentry: since those tools no longer check for certain things AND since mkiocccentry now searches under $PATH.
If we do that we could also do txzchk even though there were no functionality changes (well not really - just minor things to fit into the new interface of find_utils() for example). That is not really necessary though.

Please make those version changes that you deem appropriate.

Thanks. Will do once you address the below question.

However, please note the repo version changes to CHANGES.md(and the related change to soup/version.h). We have only one pending formal release of this repo.

In other words don't change it? Or something else?

Also, @xexyl, you might notice that PR #1239 failed.

That was my fault. I thought I had pulled changes prior to sequencing exit codes. I already fixed it.

@lcn2
Copy link
Contributor

lcn2 commented Mar 11, 2025

Well: mkiocccentry and chkentry: since those tools no longer check for certain things AND since mkiocccentry now searches under $PATH.
If we do that we could also do txzchk even though there were no functionality changes (well not really - just minor things to fit into the new interface of find_utils() for example).

Perhaps it would be worthwhile for you, @xexyl, to change the versions for mkiocccentry(1), txzchk(1), and chkentry(1) now. In case we counter an issue "out in the field" (as the expression goes) we will be able to know if the tool in question is pre or post recent update.

As we are away from our computer keyboard (where can check or make any related test changes that might be needed), please make and test the above mentioned version changes @xexyl if you please.

@xexyl
Copy link
Contributor Author

xexyl commented Mar 11, 2025

Well: mkiocccentry and chkentry: since those tools no longer check for certain things AND since mkiocccentry now searches under $PATH.
If we do that we could also do txzchk even though there were no functionality changes (well not really - just minor things to fit into the new interface of find_utils() for example).

Perhaps it would be worthwhile for you, @xexyl, to change the versions for mkiocccentry(1), txzchk(1), and chkentry(1) now. In case we counter an issue "out in the field" (as the expression goes) we will be able to know if the tool in question is pre or post recent update.

As we are away from our computer keyboard (where can check or make any related test changes that might be needed), please make and test the above mentioned version changes @xexyl if you please.

I have to think of another way to test it. Will think on it. I do know i tested it before though by manually updating the versions: before and earlier and the same and all was good. I guess I'll do that again after changing the versions.

@xexyl
Copy link
Contributor Author

xexyl commented Mar 11, 2025

Done.

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.

2 participants