lucene-solr
lucene-solr copied to clipboard
SOLR-14900: Reference Guide build cleanup/module upgrade
Description
buildSite task was generating some scary warnings on first build as it was getting various gems as well as a warning on each build. It was version locked before due to ant and PDF, but that's no longer part of Solr 9.
Solution
Upgraded all explicit versions to the latest available. Everything seems to build the same, the only changes were for the source block styles. We were supposed to be using thankful-eyes theme but the correct name of that theme is actually thankful_eyes and it is a dark theme, so we were falling back to 'github' theme. Made that more clear and explicit. Switched to using stylesheets as well instead of embedded styles, as upgrade module was generating more markup for better dark-themes support (I guess).
There is still an old warning during build about Illegal reflective access and a new one about Improper use of Lexer#lex . It may be worth repeating the upgrade process in another couple of months to see if that had been fixed in relevant libraries.
Tests
There were no code changes, but I visually and 'diff' checked the old and new html output.
Checklist
Please review the following and check all that apply:
- [X I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
- [X] I have created a Jira issue and added the issue ID to my pull request title.
- [X] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
- [X] I have developed this patch against the
master
branch. - [X] I have run
./gradlew check
.
@dweiss Can we add all my changes to that PR/JIRA (which one?) and then we can obsolete this one. Or I can wait for it to be done and do the difference. Whatever works.
It's SOLR-14889. I think it may automatically merge (because those lines are identical) but it's hard to tell in advance. Just wanted you to know Chris is working on some cleanups independently.
Ok, I see it. Looks like Chris's work should go in first. I will sit on mine for a week and then adjust to whatever still needs to be done. There is no urgency, it was just a side-effect of another research.
Thank you for the heads up.
The Jekyll 3 to 4 upgrade is a major one and has not been done to date because it has caused failures in the past when attempted. I don't understand from this so far what testing has been done to ensure it is 100% working properly - every single page was compared visually and with 'diff'? And they were exactly the same?
Thank you for the feedback. Was there a particular methodology you were going to utilize for the upgrade yourself? I did visual and diff spot checks on pages that seemed to have significant changes and only found the highlighter differences (styles and additional markup around white-spaces). But, if there are expectations of significant discrepancies, I am happy to follow a different suggested approach.
Let me build with the patch and see if I want to revise my thoughts on it. It will take me until sometime tomorrow or Thursday to have time to look at it, just FYI.
What happened in the past when I attempted to move to Jekyll 4 is that the .adoc files just simply wouldn't be converted to HTML - it would say there were no files to convert. This is what happened when Jenkins moved to Cloudbees and the wrong version caused the Jenkins build for branch_8x to fail for 2 months until Infra fixed the gemset installed on the nodes to install specifically Jekyll 3.5.2.
That seems to be working fine now here because you (I think inadvertently because you didn't mention reading the Jekyll docs on migrating from 3 to 4) hit upon the cause of the problem which was the way the jekyll-asciidoc
plugin in _config.yml
was defined: using gems
to define it was deprecated and that deprecation was removed in Jekyll 4 so the only supported way to do it is now plugins
. Which is what this PR (and Hoss' patch from SOLR-14889, now in master) changes it to. This all makes sense - Jekyll doesn't know anything about .adoc files without the plugin.
I did read the migration docs for Jekyll and I don't see anything that we should be concerned about that should require more extensive review before making the change. Knowing why it failed before and why it isn't now makes me feel easier about upgrading entirely.
Re: the syntax highlighter styling. I tested both ways of doing it, and noticed a small difference in overall size (1Mb) and makes the HTML a bit simpler if we include it as a CSS file. By the same token, as I mentioned that CSS is something we have to maintain going forward. But I am leaning toward being OK with going the CSS route. I think, though, that it needs some attribution in the CSS file itself - license/copyright info and comments about where we got it so it's clear where to go in the future when it needs to be updated.
Hoss's changes from SOLR-14889 are committed to master
now, so this needs to be updated to take out the stuff you're doing that is now already in master
.
Inadvertently is right. I was just working backwards from the latest versions and fixing warnings and it worked except for theme names. I checked the source and visually and did not see any serious problems. I did not even realize it was such a significant issue before. I guess the time helped those 3rd party packages to smooth the edges.
I'll redo this on top of Hoss's changes shortly. Will also do a short comment in css file. Not sure what to do about the license though, it is generated by the tool and does not seem to be licensed in any way itself.
Not sure what to do about the license though, it is generated by the tool and does not seem to be licensed in any way itself.
The license and copyright would belong to the Rouge project, so you could add the copyright notice as in the license at https://github.com/rouge-ruby/rouge/blob/master/LICENSE and a link to the project. Then a note about how the CSS was generated (or link to docs on it) as helper tips for anyone who comes after you who wants to re-generate the CSS.
I don't think we need to include the license with Lucene/Solr's license framework - we're not shipping a copy nor a "significant" portion of Rouge, just one CSS file. IMO providing the copyright is fine.
There are two new warnings I noticed being output with these changes: Improper use of Lexer#lex - this method does not receive options. This will become an error in a future version.
This is a Rouge error.
We don't get verbose output with the Gradle build, so I ran Jekyll locally without Gradle to see if I could catch where these occur. They both happen when processing response-writers.adoc
. If I had to guess, my first one would be that Rouge is balking at the Ruby code and misinterpreting it as inputs for itself. I have no idea if there are syntax shortcuts in that example or if it's really real Ruby...it's not making anything look any different than what it always has, though.
I thought this was kind of general error. Now that I know it is a specific file, I will dig in deeper and see if it can be eliminated.
The warning AFAIK is about the method that used to silently accept extra parameters and now is doing it loudly and threatens to quit in the future. So, the output may not change. But it is annoying to replace two old warnings with one new. So, if we can figure it out and remove it, that would be nice.
Ok, I reverted back to using direct styles. It only affects code-blocks, so the size changes are minimal and not needing to maintain the CSS file is a bonus. I agree.
Looking at generated code, the only differences I see is some additional highlighting of options and changing in order of code/pre html elements which seems to sometimes lead to removal of one level of pre/code (I suspect it was a bug before).
The warning about Lexer#lex is with source,php blocks in the file, due to asciidoctor/asciidoctor#3336, fixed in the master (not yet released). We can either ignore it or switch formatting for those two blocks to text. I think ignoring it is fine.
+1 this looks good. I think the solr/solr-ref-guide/README.adoc
also needs to be updated, though. This just occurred to me today, sorry to add another thing. It's really out of date since the move to Gradle so if you prefer to go ahead with merging this I can pick up fixing the README separately. Up to you.
When I ran Jekyll locally in my earlier work with this, I needed to upgrade Ruby to 2.7 (the one that came with my OS was 2.3.x). Jekyll 3 does not run on Ruby 2.7, so if we don't make parallel changes in branch_8x for the Ant build, we'll get ourselves stuck when we need to do local builds for 8.x releases of the Ref Guide (it's been a pain today!). I'll take a stab at a new PR on this same SOLR-14900 for that.