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

Honor nested path in tagspath #569

Merged
merged 3 commits into from Jan 15, 2019

Conversation

Projects
None yet
5 participants
@maxandersen
Copy link
Contributor

maxandersen commented Nov 13, 2018

This is a first cut on fixing #563.
Looking on feedback on where to actually put the utility method to
calculate relative path.

Why:

  • when using a path like /blogs/tags for tag.path result in wrong
    content root. its always ../

This change addreses the need by:

  • move utility method to calculate relative root to a place that
    can be shared
  • call this method during tags generation.

TODO: the getPathToRoot should honor uirExtension prefix.

Fixes #563

Honor nested path in tagspath
Why:

 * when using a path like /blogs/tags for tag.path result in wrong
   content root. its always ../

This change addreses the need by:

 * move utility method to calculate relative root to a place that
   can be shared
 * call this method during tags generation.

TODO: the getPathToRoot should honor uirExtension prefix.
@maxandersen

This comment has been minimized.

Copy link
Contributor Author

maxandersen commented Nov 13, 2018

hmm - any easy way of seeing what failed on those CI builds ? my browser is dying trying to scroll to the bottom of the log output :)

@lefou

This comment has been minimized.

Copy link
Member

lefou commented Nov 13, 2018

You can try to download the raw log on travis.

> Task :jbake-core:test FAILED

217 tests completed, 5 failed

The failings tests are (mostly) OrientDB related.

@ancho

This comment has been minimized.

Copy link
Member

ancho commented Nov 13, 2018

Do the tests fail locally, too? Otherwise it may help to purge the travis build cache.

@maxandersen

This comment has been minimized.

Copy link
Contributor Author

maxandersen commented Nov 14, 2018

I'll try run the tests again locally - but from what I can see these failures are totally unrelated to the change.

I don't think I have access to purge anything on travis ;/

@maxandersen

This comment has been minimized.

Copy link
Contributor Author

maxandersen commented Nov 14, 2018

okey, so seems I at least have 5 during renderTagsIndex tests that is causing problems. investigating ;)

@maxandersen

This comment has been minimized.

Copy link
Contributor Author

maxandersen commented Nov 14, 2018

so i figured out what is causing the failure, haven't found solution yet.

Problem is that somehow the paths used in the config are from the actual filesystem but the paths used in paths during tags rendering are on the virtual storage somehow. Resulting in the relative path calculation being confused.

i.e.
content is resolving to /Users/max/code/jbake/jbake-core/build/resources/test/fixture/content
but tags folder is created to be /var/folders/yv/ps38s9f52zdgs9wnh8hl8gz80000gn/T/junit6577498263772312109/tags
thus the relative path currently ends up with ../../../../../../../

somewhere the paths are not setup right.

@ancho

This comment has been minimized.

Copy link
Member

ancho commented Nov 14, 2018

Hi Max,

that's normal behaviour. The tests are mostly integration tests which simulate a jbake project within a temporary folder.

See setup Method in the AbstractTemplateEngineRenderingTest which extends ContentStoreIntegrationTest.

In this case the source, template and asset folder points to the location where the fixtures are. And the destination points to the temporary folder.

The destination is per default relative to the project structure. But it is possible to put the output where ever you want. The cli let you specify the destination somewhere else too.

@ancho

This comment has been minimized.

Copy link
Member

ancho commented Nov 14, 2018

As an addition, you do not deploy the source, just the output. So everything you do with paths during the rendering or better when the sourcefiles get converted from the concrete engines, should refer to the output path as it is the document root of your homepage.

@maxandersen

This comment has been minimized.

Copy link
Contributor Author

maxandersen commented Nov 14, 2018

yeah, I found a place where I swapped destination with source that caused problems. pushed a change that passes locally - lets see how that goes.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 14, 2018

Coverage Status

Coverage increased (+0.009%) to 80.807% when pulling d5807d5 on maxandersen:pr563 into aebf496 on jbake-org:master.

2 similar comments
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 14, 2018

Coverage Status

Coverage increased (+0.009%) to 80.807% when pulling d5807d5 on maxandersen:pr563 into aebf496 on jbake-org:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 14, 2018

Coverage Status

Coverage increased (+0.009%) to 80.807% when pulling d5807d5 on maxandersen:pr563 into aebf496 on jbake-org:master.

@jonbullock jonbullock added this to the v2.6.4 milestone Dec 8, 2018

@jonbullock jonbullock self-assigned this Dec 8, 2018

@jonbullock jonbullock added the bugfix label Dec 8, 2018

@ancho
Copy link
Member

ancho left a comment

Thank you. And sorry for the late review. I have noticed that some tests fail on appveyor.
Do you want to make the change I proposed in the code comment?


StringBuilder sb = new StringBuilder();

sb.append(relativePath.toString());

This comment has been minimized.

@ancho

ancho Jan 12, 2019

Member

On Windows the relative path contains "\" which is wrong in context of an URI for image paths.
Causing some tests on appveyor to fail. https://ci.appveyor.com/project/jbake-org/jbake/builds/20288027

org.jbake.app.configuration.ConfigUtilTest > testGetContentRoothPath FAILED
    org.junit.ComparisonFailure: expected:<"..[/]../"> but was:<"..[\]../">
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at org.jbake.app.configuration.ConfigUtilTest.testGetContentRoothPath(ConfigUtilTest.java:352)

We should replace alle backslashes with slashes here.

This comment has been minimized.

@ancho

ancho Jan 12, 2019

Member

Maybe the methods should be moved to FileUtil and contain a pointer that we get a URI Path back? Something like getUriPath.... ?

return sb.toString();
}

static public String getPathtoDestinationRoot(JBakeConfiguration config, File sourceFile) {

This comment has been minimized.

@ancho

ancho Jan 12, 2019

Member

Little typo getPathtoDestinationRoot -> getUriPathToDestinationRoot. What do you think?

@maxandersen

This comment has been minimized.

Copy link
Contributor Author

maxandersen commented Jan 13, 2019

Not trying to use jbake anymore - sorry - feel free to close this or push fixes to it as you see fit.

@ancho

This comment has been minimized.

Copy link
Member

ancho commented Jan 13, 2019

All right. Sorry to hear that. Thank you for the answer.

@ancho ancho merged commit d5807d5 into jbake-org:master Jan 15, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment