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.

Sign up for GitHub

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

Jump to bottom

Fix #8: Incremental building not working #18

Merged
merged 1 commit into from
Apr 27, 2021
Merged

Fix #8: Incremental building not working #18

merged 1 commit into from
Apr 27, 2021

Conversation

terencehonles
Copy link

This incorporates my suggestions in #14 and is just a PR to make #14 (comment) easier

@terencehonles terencehonles mentioned this pull request Jan 6, 2021
Closed
@jonascj
Copy link
Collaborator

jonascj commented Jan 6, 2021

Let me see if I can get this reviewed and tested this coming weekend (9th, 10th January). Thank you for moving it forward @terencehonles

See also #20 :-)

jonascj
jonascj reviewed Jan 10, 2021
View reviewed changes
Copy link
Collaborator

@jonascj jonascj left a comment

Choose a reason for hiding this comment

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

Thank you for your suggested solution. It does resolve the issue and removes some unnecessary lines. Please see my individual comments (which are questions as much as comments).

sphinxcontrib/images.py Outdated
@@ -265,13 +265,15 @@ def download_images(app, env):
.format(src, dst))


def configure_backend(app):
global DEFAULT_CONFIG
def check_config(app, config):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure check_config() is the best name for this function since it does not check anything, it updates a dictionary afaik. configure_backend() is not a good name either, though, you are right. Maybe update_config()?

Copy link
Author

Choose a reason for hiding this comment

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

I'm fine with update_config since it validates it and updates it as needed.

sphinxcontrib/images.py
@@ -265,13 +265,15 @@ def download_images(app, env):
.format(src, dst))


def configure_backend(app):
global DEFAULT_CONFIG
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary yes, we only read the DEFAULT_CONFIG dictonary.

sphinxcontrib/images.py
Comment on lines -271 to +273
config = copy.deepcopy(DEFAULT_CONFIG)
config.update(app.config.images_config)
app.config.images_config = config
merged = copy.deepcopy(DEFAULT_CONFIG)
merged.update(config.images_config)
config.images_config = merged
Copy link
Collaborator

Choose a reason for hiding this comment

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

So what is the rationale behind changing config as opposed to app.config?

In configure_backend(app) we get the images_config dictionary from config = app.config.images_config, so the goal of check_config(app, config) must be to update app.config, otherwise the updates would not be available in configure_backend(app) which only takes app as parameter...

Copy link
Author

Choose a reason for hiding this comment

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

I assume this is just inconsistency in Sphinx but app.config is config, but since config is passed as a parameter here I figured it would make sense to use the parameter, otherwise it's unused.

sphinxcontrib/images.py
@@ -343,9 +345,9 @@ def inner_wrapper(writer, node):
app.env.remote_images = {}

def setup(app):
global DEFAULT_CONFIG
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary yes.

sphinxcontrib/images.py
Comment on lines -348 to +349
app.add_config_value('images_config', DEFAULT_CONFIG, 'env')
app.add_config_value('images_config', {}, 'env')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the current suggestion completely recreates the config-object (in check_config(app, config)) we might as well supply an empty dictionary as default value.

It would be pretty however, wouldn't it, to pass in the defaults here? But maybe this is the origin of the problem/issue. Our multiple configurations options are written in the images_config dictionary (as opposed to several individual configuration variables), and somehow someone in the past got the copying/updating of the dictionary wrong...

Copy link
Author

Choose a reason for hiding this comment

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

This line is just registering a dictionary with Sphinx so that any user supplied config can land there. In the check_config it validates that the config has all the properties that are expected later.

@jonascj
Copy link
Collaborator

jonascj commented Jan 10, 2021 •
edited
Loading

@terencehonles I've reviewed your changes and asked you (or just aloud) a few questions.

I can't shake the feeling that somewhere along the way someone just updated the config dictionary wrong (got the copy semantics wrong, maybe that is why there were a few globals). So I'll see if I can get something in between your suggestions and @kujiu's suggestion to work.

Also, we have a Travis build problem at the moment, see sphinx-contrib/github-administration#15. I'd like that resolved before I release a new version of the package (I do not have all the many Python version and implementations installed locally, so I can't run all the tests locally).

EDIT: can to can't

@jonascj
Copy link
Collaborator

jonascj commented Apr 17, 2021

@terencehonles Today the Travis support team responded, after a 3 months wait, that they've allocated me some open source build-time (they did apologize for the wait though).

So builds are running again at Travis and I'll get back on this PR (during next week).

@terencehonles
Copy link
Author

Sounds good. I haven't had time to look at this in awhile (more it wasn't high enough for the free time I had). I'll make sure the PR is up to date (so thanks for the ping) and I may be able to see about suggesting a PR for GitHub actions which has become a lot more popular lately

@terencehonles
Copy link
Author

I responded to your comments and changed the name of the function as requested.

@jonascj jonascj merged commit 785472b into sphinx-contrib:master Apr 27, 2021
@terencehonles terencehonles deleted the master-fix-incremental-build branch April 28, 2021 00:59
@SilverRainZ
Copy link

Does this PR actually fix #8?

I am using sphinx 4.0.2 with sphinxcontrib-image 0.9.3, and it always say 'config changed':

Running Sphinx v4.0.2
loading translations [zh_CN]... done
loading pickled environment... done
Initiated sphinxcontrib-images backend: `sphinxcontrib_images_lightbox2.lightbox2:LightBox2`
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 0 source files that are out of date
updating environment: [config changed ('images_config')] 124 added, 10 changed, 0 removed

@terencehonles
Copy link
Author

Replied in #8, and let's try to keep the discussion in one place

@terencehonles terencehonles mentioned this pull request Jun 16, 2021
Closed
@terencehonles
Copy link
Author

sorry I had too many tabs open and looks like I put it on the wrong one 🤦 it's fixed now

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

@jonascj jonascj jonascj left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@terencehonles @jonascj @SilverRainZ @kujiu