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

fixes #1599 #1672

Closed
wants to merge 1 commit into from
Closed

fixes #1599 #1672

wants to merge 1 commit into from

Conversation

mtolk
Copy link
Contributor

@mtolk mtolk commented Dec 1, 2021

no longer associate assets from subfolders, only the current folder.
This caused two threads writing creating the same file. when this happens at the same time zola build on windows crashes.

I've ran the test-suite about 10 times no longer seen the error.

IMPORTANT: Please do not create a Pull Request adding a new feature without discussing it first.

The place to discuss new features is the forum: https://zola.discourse.group/
If you want to add a new feature, please open a thread there first in the feature requests section.

Sanity check:

  • [ x ] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

  • [ x ] Are you doing the PR on the next branch?

no longer associate assets from subfolders, only the current folder
@Keats
Copy link
Collaborator

Keats commented Dec 2, 2021

cc @timakro looking at that change it does make sense to me to ignore the subdirectories and it wouldn't break anything, correct?

@Keats Keats mentioned this pull request Dec 2, 2021
@timakro
Copy link
Contributor

timakro commented Dec 3, 2021

@mtolk @Keats On first glance it seems to me this effectively reverts #1582. Changing the asset scanning from flat (read_dir) to recursive (WalkDir) was the first step to making subfolder colocation work. Note also that I added two dummy files in the tests to specifically test subfolder colocation, increasing the number of assets from 3 to 4, which this PR reverts as well. I might be missing something; only way to know for sure is testing.

@Keats
Copy link
Collaborator

Keats commented Dec 3, 2021

It looks like it still works: #1597 (comment)

But I see the issue. The issue is that section assets are poorly defined so using walkdir on a section would just mark every single page asset as their own.
I think the right fix is to use Walkdir for pages and read_dir for sections? Honestly at some point we should just ignore assets in the same folder as the section and force the use of a folder like we do for pages.

@Keats
Copy link
Collaborator

Keats commented Dec 3, 2021

I think #1677 might work?

@eberkund
Copy link

eberkund commented Dec 3, 2021

Do I understand correctly that the difficulty with this feature is that it's a little tricky to figure out how a file should be treated because it differs based on (not sure which factors?)

@Keats
Copy link
Collaborator

Keats commented Dec 3, 2021

So a section and a page can have colocated assets. A page has to be in its own folder while assets for a sections are at its level (not consistent, it will need to be fixed, in an ulterior version).
Imagine you have the following folders:

content
   - blog
      - index.md
      - banner.gif
      -  an_article
          - index.md
          - image.jpg
          -  example_js
            - data.js

When looking for the assets for the an_article page, it can search recursively in the folder thanks to @timakro PR since it's a page, there's nothing below in terms of content.
However, if you do a recursive search for assets for the blog section, it will find banner.gif but also all the assets from every article below which would cause some files to be copied several times (twice or more, it depends if you have subsection).
I believe #1677 solves that by only searching recursively if we are looking for the assets of a page.

@eberkund
Copy link

eberkund commented Dec 3, 2021

Gotcha, so this could potential break if you had a subfolder of blog called blog_assets with perhaps banner.gif inside it right? I only tested the page scenario.

I think we need to check if the subfolders do not contain an index.md file then the folder can be treated as a colocated asset of the parent otherwise it would be treated as a page or section.

@Keats
Copy link
Collaborator

Keats commented Dec 3, 2021

this could potential break if you had a subfolder of blog called blog_assets with perhaps banner.gif inside it right?

This is more an issue with duplicated assets in the output dir and threads trying to copy the files at the same time which seems to be an issue on Windows.

@mtolk
Copy link
Contributor Author

mtolk commented Dec 4, 2021

The problem i tried to solve was that, because 1 file could be an asset of multiple sections and a page, it was copied multiple times. It's actually the multiple copies that cause erros that was my problem.

I think it makes sense that an asset is associated with only 1 page or section, but that's not a requirement i think. I used it as a solution to the multiple copying problem.

If you would allow different pages and section to share an assets another solution would have to found to prevent it being copied multiple times, at the same time.

#1677 is already a more elegant solution that this pull request. I had never realised people would place assets in subfolders.

@Keats
Copy link
Collaborator

Keats commented Dec 4, 2021

I had never realised people would place assets in subfolders.

It's a new feature, it hasn't been released yet ;)

I'll close this in favour of #1677 .
@timakro if you can try the next branch again that would be great, it's good to release after that I think.

@Keats Keats closed this Dec 4, 2021
@mtolk mtolk deleted the issue1599 branch December 7, 2021 09:37
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.

4 participants