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

fix build error and add enhancements #2

Merged
merged 61 commits into from
Apr 1, 2024
Merged

fix build error and add enhancements #2

merged 61 commits into from
Apr 1, 2024

Conversation

dariusptrs
Copy link
Member

  • fix build error: Running git config --global --add safe.directory $GITHUB_WORKSPACE before the build allows cmake to write into 'git.id' and fixes the following build error:
./CheatsheetTemplate.tex:62: Undefined control sequence.
\themydate ->\GitNiceDate 
                          \ (git \GitRevision )
l.62 \end{document}
  • Suggestion: add a "Update README and LaTeX Build File" workflow to make using the template easier

  • Suggestion: add a hyperlink into every document that directly forwards the user to the repo's issues page. (For that also see my PR in latex4ei-packages here: Add issues hyperlink and fix CI latex4ei-packages#6)

dariusptrs and others added 6 commits March 21, 2024 23:25
Test commit

Update README and LaTeX build configuration with correct repository name

Update README.md

Test commit

Update README and LaTeX build configuration with correct repository name

update .gitignore
add main branch to ci.yml

add main branch to ci.yml

Update README.md and tests

Revert "Update README and LaTeX build configuration with correct repository name"

This reverts commit b8d6caf.

Update README and LaTeX build configuration with correct repository name

Update update.yml

Update README.md

Update README and LaTeX build configuration with correct repository name

Update update.yml

Update README and LaTeX build configuration with correct repository name

Update update.yml

no additional '/workflows/CI' at the end

Update update.yml to change project name in CMakeLists.txt

Update update.yml to change project name in CMakeLists.txt

Update README and LaTeX build configuration with correct repository name

Update update.yml

Update update.yml

Update update.yml

Update README and LaTeX build configuration with correct repository name

Update README and LaTeX build configuration with correct repository name

Update update.yml

Update README.md

Update README and LaTeX build configuration with correct repository name

Update update.yml

Update README and LaTeX build configuration with correct repository name

Update update.yml

no additional '/workflows/CI' at the end
add issues url

add condition to check for setissueslinkurl cmd
Revert "Update README and LaTeX build configuration with correct repository name"

This reverts commit 87b70c8.
@dariusptrs dariusptrs marked this pull request as ready for review March 26, 2024 19:31
Copy link
Member

@hofbi hofbi left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions! Just a few comments to be discussed

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CheatsheetTemplate.tex Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
write-gitid.sh Outdated Show resolved Hide resolved
.github/workflows/update.yml Outdated Show resolved Hide resolved
Copy link
Member

@hofbi hofbi left a comment

Choose a reason for hiding this comment

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

Thanks for your efforts so far. Just a few more comments for the python code

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/update.yml Outdated Show resolved Hide resolved
.github/workflows/update.yml Outdated Show resolved Hide resolved
.github/workflows/update.yml Outdated Show resolved Hide resolved
.github/workflows/update.yml Outdated Show resolved Hide resolved
.github/workflows/update.yml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@dariusptrs
Copy link
Member Author

I just did some tests with the new workflow. Everything seems to work.

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/update.yml Outdated Show resolved Hide resolved
scripts/update_files.py Outdated Show resolved Hide resolved
scripts/update_files.py Outdated Show resolved Hide resolved
scripts/update_files.py Outdated Show resolved Hide resolved
scripts/update_files.py Outdated Show resolved Hide resolved
scripts/update_files.py Outdated Show resolved Hide resolved
scripts/update_files.py Outdated Show resolved Hide resolved
scripts/update_files.py Outdated Show resolved Hide resolved
scripts/update_files.py Outdated Show resolved Hide resolved
dariusptrs and others added 2 commits March 28, 2024 18:53
Co-authored-by: Markus Hofbauer <[email protected]>
Co-authored-by: Markus Hofbauer <[email protected]>
Copy link
Member

@hofbi hofbi left a comment

Choose a reason for hiding this comment

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

Almost good.

Another cool thing we could do is to create some unit tests for our Python script (can be a follow up PR, if you want). For this, I would recommend pytest and its fakefilesystem plugin. This makes it super easy to unit test file io operations without writing to the disk.

.gitattributes Outdated Show resolved Hide resolved
.github/workflows/update.yml Outdated Show resolved Hide resolved
scripts/update_files.py Outdated Show resolved Hide resolved
scripts/update_files.py Outdated Show resolved Hide resolved
scripts/update_files.py Outdated Show resolved Hide resolved
@dariusptrs
Copy link
Member Author

I tried to add the unit tests. The test now only fails for the CMakeLists.txt. I can look into that later, or maybe you also find the mistake until then.

@dariusptrs
Copy link
Member Author

I tried to add the unit tests. The test now only fails for the CMakeLists.txt. I can look into that later, or maybe you also find the mistake until then.

E       AssertionError: CMakeLists.txt content was not updated correctly.
E       assert 'cmake_minimum_required(VERSION 3.12)\nproject(correct_title Add the main LaTeX correct_title.tex\nFORCE_PDF\nIMAGE_DIRS img\nDEPENDS writegitid\n)\n' == 'cmake_minimum_required(VERSION 3.12)\nproject(correct_title)\n# Add the main LaTeX document\nadd_latex_document(\ncorrect_title.tex\nAnotherDoc.tex\nFORCE_PDF\nIMAGE_DIRS img\nDEPENDS writegitid\n)\n'
E         
E           cmake_minimum_required(VERSION 3.12)
E         + project(correct_title Add the main LaTeX correct_title.tex
E         - project(correct_title)
E         - # Add the main LaTeX document
E         - add_latex_document(
E         - correct_title.tex
E         - AnotherDoc.tex
E           FORCE_PDF
E           IMAGE_DIRS img
E           DEPENDS writegitid
E           )

/Users/darius/Documents/GitHub/CheatsheetTemplate/scripts/tests/test_update_files.py:77: AssertionError
=========================== short test summary info ============================
FAILED scripts/tests/test_update_files.py::test_update_cmake_content - AssertionError: CMakeLists.txt content was not updated correctly.

@dariusptrs
Copy link
Member Author

I tried to add the unit tests. The test now only fails for the CMakeLists.txt. I can look into that later, or maybe you also find the mistake until then.

E       AssertionError: CMakeLists.txt content was not updated correctly.
E       assert 'cmake_minimum_required(VERSION 3.12)\nproject(correct_title Add the main LaTeX correct_title.tex\nFORCE_PDF\nIMAGE_DIRS img\nDEPENDS writegitid\n)\n' == 'cmake_minimum_required(VERSION 3.12)\nproject(correct_title)\n# Add the main LaTeX document\nadd_latex_document(\ncorrect_title.tex\nAnotherDoc.tex\nFORCE_PDF\nIMAGE_DIRS img\nDEPENDS writegitid\n)\n'
E         
E           cmake_minimum_required(VERSION 3.12)
E         + project(correct_title Add the main LaTeX correct_title.tex
E         - project(correct_title)
E         - # Add the main LaTeX document
E         - add_latex_document(
E         - correct_title.tex
E         - AnotherDoc.tex
E           FORCE_PDF
E           IMAGE_DIRS img
E           DEPENDS writegitid
E           )

/Users/darius/Documents/GitHub/CheatsheetTemplate/scripts/tests/test_update_files.py:77: AssertionError
=========================== short test summary info ============================
FAILED scripts/tests/test_update_files.py::test_update_cmake_content - AssertionError: CMakeLists.txt content was not updated correctly.

Still found some time. I think it's fixed now.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can merge this into the ci.yml. Let's put it between the checkout and the git add safe dir step.

Since it is super fast and we don't have that many changes, we don't need the extra path/changes logic.

.github/workflows/test_update.py.yml Outdated Show resolved Hide resolved
scripts/tests/test_update_files.py Outdated Show resolved Hide resolved
scripts/tests/test_update_files.py Outdated Show resolved Hide resolved
scripts/tests/test_update_files.py Outdated Show resolved Hide resolved
scripts/tests/test_update_files.py Outdated Show resolved Hide resolved
scripts/tests/test_update_files.py Outdated Show resolved Hide resolved
scripts/tests/test_update_files.py Outdated Show resolved Hide resolved
scripts/tests/test_update_files.py Outdated Show resolved Hide resolved
scripts/tests/test_update_files.py Outdated Show resolved Hide resolved
@dariusptrs
Copy link
Member Author

I removed the hours and minutes from the Last revised because I think it is unnecessary and we have \GitRevision to track the exact commit.

@hofbi hofbi merged commit 91d709d into latex4ei:master Apr 1, 2024
2 checks passed
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.

2 participants