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

mkhtml: Fix print warning/fatal message during compilation #2139

Conversation

tmszi
Copy link
Member

@tmszi tmszi commented Jan 28, 2022

Fixes #2137, #2138. Suggested by #2137 (comment).

Note:

Git repository build: core modules manuals have git commit info

Non Git repository build: some core modules manuals have git commit info and some haven't, due GitHub API server error urllib.error.HTTPError: HTTP Error 403: rate limit exceeded

@tmszi tmszi added bug Something isn't working backport_needed labels Jan 28, 2022
@neteler neteler added this to the 8.0.1 milestone Jan 28, 2022
@nilason
Copy link
Contributor

nilason commented Jan 29, 2022

This still fails on the _:

gs.warning(
            _(
)

part.

Traceback (most recent call last):
  File "/Users/nilason/Downloads/grass-8.0.0/dist.x86_64-apple-darwin21.2.0/utils/mkhtml.py", line 152, in download_git_commit
    response = urlopen(url, *args, **kwargs)
  File "/Users/nilason/Downloads/grass-8.0.0/dist.x86_64-apple-darwin21.2.0/utils/mkhtml.py", line 124, in urlopen
    return urlrequest.urlopen(request, *args, **kwargs)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/urllib/request.py", line 214, in urlopen
    return opener.open(url, data, timeout)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/urllib/request.py", line 523, in open
    response = meth(req, response)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/urllib/request.py", line 632, in http_response
    response = self.parent.error(
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/urllib/request.py", line 561, in error
    return self._call_chain(*args)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/urllib/request.py", line 494, in _call_chain
    result = func(*args)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/urllib/request.py", line 641, in http_error_default
    raise HTTPError(req.full_url, code, msg, hdrs, fp)
urllib.error.HTTPError: HTTP Error 403: rate limit exceeded

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/nilason/Downloads/grass-8.0.0/dist.x86_64-apple-darwin21.2.0/utils/mkhtml.py", line 669, in <module>
    git_commit = get_last_git_commit(
  File "/Users/nilason/Downloads/grass-8.0.0/dist.x86_64-apple-darwin21.2.0/utils/mkhtml.py", line 243, in get_last_git_commit
    response = download_git_commit(
  File "/Users/nilason/Downloads/grass-8.0.0/dist.x86_64-apple-darwin21.2.0/utils/mkhtml.py", line 180, in download_git_commit
    _(
NameError: name '_' is not defined

@nilason
Copy link
Contributor

nilason commented Jan 29, 2022

Following seem to work:

try:
    import grass.script as gs
except ImportError:
    # During compilation GRASS GIS
    _ = str
    class gs:
        def warning(message):
            pass

        def fatal(message):
            pass

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Given that no translation is needed during compilation _ = str on import error is the shortest way of making it work as @nilason suggests.

The warning function can pass or print. I don't know what is more appropriate, but if it can be pass, then I wonder if it is just a verbose message rather than a warning.

The fatal function should be at least print as in the PR (just pass as in @nilason's snippet is really "ignore all errors"). However, since it is fatal, that usually means in GRASS GIS ending the process immediately. You can do exit or raise an exception like we do grass.script.core when the library is configured that way.

Also, the messages would be prefixed by "ERROR: " if that would be the library function, so it makes sense to me to keep that behavior.

See also grass.py where similar code is needed.

@nilason
Copy link
Contributor

nilason commented Jan 29, 2022

Print gives new error: #2137 (comment)

Also why would a HTTPError need to break compilation (with no git)?

@wenzeslaus
Copy link
Member

Print gives new error: #2137 (comment)

Maybe I'm missing something, but if printing itself gives an error, than that the printing should be fixed. If there is a subsequent error, than it means the code does not deal with a perfectly valid state. Or, that there is fatal/exit used where only a warning or verbose message should be.

Also why would a HTTPError need to break compilation (with no git)?

I don't think it should. Compilation should work offline and without Git. Hence my suggestion to consider making some of these verbose messages (i.e., not printed by default).

@tmszi
Copy link
Member Author

tmszi commented Jan 29, 2022

Alternatively I created a new PR #2140 (which solves this problem more comprehensively), where I replaced getting Git commit from remote GitHub API server during compilation (for core modules) with getting Git commit from a locally created/updated "pgms_with_last_commit.json" file (with GitHub action), and which is part of source code.

@wenzeslaus
Copy link
Member

#2140 is hopeful, but potentially complex, I think 8.2+ material, so this PR would be to get a solution for 8.0.1.

@nilason
Copy link
Contributor

nilason commented Jan 29, 2022

Print gives new error: #2137 (comment)

Maybe I'm missing something, but if printing itself gives an error, than that the printing should be fixed. If there is a subsequent error, than it means the code does not deal with a perfectly valid state. Or, that there is fatal/exit used where only a warning or verbose message should be.

I'm not too familiar with this code, but I think this function call is inside an ongoing stream (building the html) therefore the print() doesn't work there. gs.warning is output later.

Also why would a HTTPError need to break compilation (with no git)?

I don't think it should. Compilation should work offline and without Git. Hence my suggestion to consider making some of these verbose messages (i.e., not printed by default).

Let's not forget these gs.warning and gs.fatal are only within download_git_commit(), so pass on these during compilation is perfectly ok in my opinion.

@nilason
Copy link
Contributor

nilason commented Jan 29, 2022

Just tested compiling with and without git with above suggested code #2139 (comment). Succeeded in both cases.
Please do test, for a quick fix for 8.0.1 that might be good enough.

@tmszi
Copy link
Member Author

tmszi commented Jan 29, 2022

The warning function can pass or print. I don't know what is more appropriate, but if it can be pass, then I wonder if it is just a verbose message rather than a warning.

Yes I agree these warnings messages should be verbose messages.

Rather, I think we should disable the download of Git commit info from the remote GitHub API server for core modules for now in the case of non-git source code. As I mentioned, there will be a situation where some core module man pages will have git commit info and others will not.

@nilason
Copy link
Contributor

nilason commented Jan 29, 2022

As I mentioned, there will be a situation where some core module man pages will have git commit info and others will not.

Would that be a problem for the user?

@wenzeslaus
Copy link
Member

...disable the download of Git commit info from the remote GitHub API server...

It seems that you can't use GitHub API for free for this amount of requests, so will that ever work in practice? With the calls sometimes hitting the rate limit, the result is just random.

Comment on lines +54 to +58
def warning(message):
pass

def fatal(message):
pass
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing to me. Why this is not verbose message in the first place? This is meant primary for compilation anyway, no? When you don't want the things to print ERROR:... and exit or print WARNING:..., then don't call gs.warning and gs.fatal. When reading the other part of the code, I will not remember that in this file the fatal function is redefined to do nothing.

Copy link
Member Author

Choose a reason for hiding this comment

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

My original intention was to display these warning / fatal messages during the addon installation via g.extension, (then the remote GitHub API server is called) and not to display these messages during compilation.

Copy link
Member Author

Choose a reason for hiding this comment

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

download_git_commit() function is called:

  1. During GRASS GIS compilation, when Git repo doesn't exists
  2. Addon installation (via g.extension module)

@tmszi
Copy link
Member Author

tmszi commented Jan 29, 2022

As I mentioned, there will be a situation where some core module man pages will have git commit info and others will not.

Would that be a problem for the user?

This can be confusing for the user because instead of the last commit date, it will have a build date depending on hit this error, urllib.error.HTTPError: HTTP Error 403: rate limit exceeded and it will also vary between GRASS GIS builded from the Git repository source code and without the Git repository. Implemented PR #2100 should be the basis for GRASS GIS Addons PR OSGeo/grass-addons#677. @ninsbl knows more about that. I don't think it's serious, but it can be confusing. That is why I am solving it in the new PR #2140. For now, we can leave it at that.

Anyway, I'm adding your suggestions, which I tested.

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

I think we can all agree, a better and more sustainable solution is needed. #2140 is a reminder of that.
If the present solution of this PR will not lead to consequences in usage, I'd suggest to go with this and make release 8.0.1 .

@tmszi tmszi merged commit 9b3fb6f into OSGeo:main Feb 7, 2022
tmszi added a commit to tmszi/grass that referenced this pull request Feb 7, 2022
tmszi added a commit to tmszi/grass that referenced this pull request Feb 7, 2022
@tmszi tmszi deleted the fix-mkhtml-get-git-commit-if-git-repo-and-net-conn-is-now-available branch February 7, 2022 12:20
@tmszi tmszi mentioned this pull request Feb 11, 2022
@wenzeslaus wenzeslaus changed the title utils/mkhtml: fix print warning/fatal message during compilation mkhtml: Fix print warning/fatal message during compilation Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Compiling version 8 fails with a no-git-repo
4 participants