Closed tdegeus closed 2 years ago
Thanks for this.
All pull requests should go to develop
branch, please.
If this only works for conda, and it relies on an issue that isn't yet fixed, then I don't think I can merge this.
I don't have any updates on the GCString issue, all updates will be posted to that thread.
On conda it should work on everything but macOS ARM. I'm fine with waiting for #303 . Or, if you think it's sufficiently safe, I could also apply the non-GCString version as patch to the conda package.
Alternatively, there is the MakeFile.PL
way but for that I would need help.
OK, many thanks.
Can we see if I can progress https://github.com/cmhughes/latexindent.pl/issues/303? It's one of my next priorities, and I'm hopeful to look at it soon.
A few questions on this, if I may:
conda
)? I'm new to this pre-commit (as of https://github.com/cmhughes/latexindent.pl/issues/316), so as much information as possible, please. I'm on Ubuntu..pre-commit-hooks.yaml
: is it a repository file, or is it a user file?latexindent.pl
with different options, how would I do so?environment.yml
: is this a conda
-specific file? if so, I'm not sure it is appropriate...?I have added documentation to answer part of your questions. In addition:
A few questions on this, if I may:
- how do I test this (not using
conda
)? I'm new to this pre-commit (as of Support usinglatexindent
as apre-commit
hook #316), so as much information as possible, please. I'm on Ubuntu.
That is relatively straightforward.
git init
, and stage the TeX file..pre-commit-config.yaml
and stage it, as follows:
repos:
- repo: https://github.com/cmhughes/latexindent.pl
rev: 3.13.5
hooks:
- id: latexindent.pl
args: []
pre-commit try-repo /path/to/clone/PR/latexindent.pl latexindent-conda --verbose --all-files
For it to work you will need a conda installation, and you will need to have pre-commit
(which you can install using conda
).
- about the file
.pre-commit-hooks.yaml
: is it a repository file, or is it a user file?
This is repository file. As a tool provider you provide some rules for pre-commit
. The user on its side will set preferences in
a file .pre-commit-config.yaml
in its repository.
- if I'm a user and I want to call
latexindent.pl
with different options, how would I do so?
Under the args
option in .pre-commit-config.yaml
, see latests commit.
- about the file
environment.yml
: is this aconda
-specific file? if so, I'm not sure it is appropriate...?
It allows pre-commit
to set up its conda environment. It enforces this specific name.
Let me know if there are other questions.
What I would have liked: a GitHub Action to check all of this continuously. I don't know how to do this though. One could have used the local run as outlined under 1. But it seems crucial to use two repos there. Also, it seems to be using the latest release, which is sort of ok, but not ideal for continuous integration.
Thanks that's helpful.
I'll review it soon.
In the meantime, it's the documentation /.tex
files that need to be updated, please. The rst files are an output, not an input. I'd like this to go in the appendix, please, after the section on conda.
I have tried on my Linux machine:
latexindent-conda
works fine.latexindent-cpan
results in FATAL Could not open defaultSettings.yaml
. That makes sense as MakeFile.PL
does nothing to it. I don't know how to solve this. I tried moving defaultSettings.yaml
to LatexIndent
but that does help.OK, many thanks.
Looks like this has diverged from develop, it'd be great if you could rebase.
I rebased and pushed, but somehow GitHub is unhappy about a conflict...? Maybe you can fix it?
OK.
But, as I review, it'll need the perl/cpan version to work.
I don't use conda, and I don't require users to use it, so having a feature that only works with it is not an option.
I hope to get to this soon....
If you'd just find a solution for the defaultSettings.yaml
I think that CPAN should work too: locally it did manage to get all the dependencies to work
Hello both and thank you @tdegeus for taking over the implementation of #316
I tried moving defaultSettings.yaml to LatexIndent but that does help.
According to my tests, no lookups are made for defaultSettings.yaml file in the LatexIndent directory. I found path lookups to be not easy to grasp (Perhaps because the project has to be compliant with the texlive distribution). Anyway, I tested your changes with copying the defaultSettings.yaml file in the LatexIndent directory and making the following change would solve the problem.
--- a/LatexIndent/GetYamlSettings.pm
+++ b/LatexIndent/GetYamlSettings.pm
@@ -61,6 +61,8 @@ sub yaml_read_settings{
# grab the logger object
$logger->info("*YAML settings read: defaultSettings.yaml");
$logger->info("Reading defaultSettings.yaml from $FindBin::RealBin/defaultSettings.yaml");
+ my $dirName = dirname(__FILE__);
# if latexindent.exe is invoked from TeXLive, then defaultSettings.yaml won't be in
# the same directory as it; we need to navigate to it
@@ -71,6 +73,8 @@ sub yaml_read_settings{
$defaultSettings = YAML::Tiny->read( "$FindBin::RealBin/../../texmf-dist/scripts/latexindent/defaultSettings.yaml");
} elsif ( -e "$FindBin::RealBin/LatexIndent/defaultSettings.yaml" ) {
$defaultSettings = YAML::Tiny->read( "$FindBin::RealBin/LatexIndent/defaultSettings.yaml");
+ } elsif ( -e "$dirName/defaultSettings.yaml" ) {
+ $defaultSettings = YAML::Tiny->read( "$dirName/defaultSettings.yaml");
} else {
$logger->fatal("*Could not open defaultSettings.yaml");
$self->output_logfile();
I don't use conda, and I don't require users to use it, so having a feature that only works with it is not an option.
I agree, conda may be a useful option for some users but it should not be a requirement. I think the docs should reflect this idea by mentioning the cpan version of the hook first (or recommending the use of cpan?).
@hamdanal I think it is a good idea to make the cpan the default (it is probably also slightly faster on initial setup), and I would even using as naming latexindent
(instead of latexindent-cpan
) and latexindent-conda
. But it would indeed need the fix of defaultSettings.yaml
. Also, it would need to be placed in LatexIndent
in the MakeFile.PL
(which I don't know how to do, but you might), or shipping it differently (which I would find very sensible as you would just have the complete default library together, but @cmhughes has probably reasons why it is not do in the first place?).
A remark though: in the conda package I do place the defaultSettings.yaml
in the LatexIndent
directory and it does work (https://github.com/conda-forge/latexindent.pl-feedstock/blob/d0bc7326dc498b6c0ea7f90aba0d7394b32309be/recipe/meta.yaml#L18 ). I remember that that was made possible recently. Could it be that it is simply not on the develop
branch?
Edit Reference : https://github.com/cmhughes/latexindent.pl/releases/tag/V3.13.2
So if this indeed possible there is thus the question how to make MakeFile.PL
instruct its include. Do you happen to know how to do that @hamdanal ? It does not explain though why my manual attempt failed.
I think it is a good idea to make the cpan the default (it is probably also slightly faster on initial setup), and I would even using as naming latexindent (instead of latexindent-cpan) and latexindent-conda
I agree. The new naming makes more sense.
Also, it would need to be placed in LatexIndent in the MakeFile.PL (which I don't know how to do, but you might)
Unfortunately I don't have any experience with perl at all so this is beyond what I can help with. I would note however that the change I made was enough to install the package with the settings file included (maybe this is the default behaviour?)
A remark though: in the conda package I do place the defaultSettings.yaml in the LatexIndent directory and it does work
It appears that you copy everything to a bin directory https://github.com/conda-forge/latexindent.pl-feedstock/blob/d0bc7326dc498b6c0ea7f90aba0d7394b32309be/recipe/meta.yaml#L16-L18 and then you have this branch https://github.com/cmhughes/latexindent.pl/blob/main/LatexIndent/GetYamlSettings.pm#L72-L73 in the code to look in that bin folder. This is probably not the default behaviour of MakeFile.PL hence the difference of behaviour.
Thanks both.
I'm a little worried about the defaultSettings.yaml issue, as it seems to be occurring for others too https://github.com/cmhughes/latexindent.pl/issues/323
I'm hopeful to review all of this soon.
In the coda package I indeed copy defaultSettings.yaml
to LatexIndent
and then the whole folder to bin/
. I guess that this could be improved in some future by copying LatexIndent
to a perl library folder (if only I would know how that works ;))
My thoughts for MakeFile.PL
were that it copies the LatexIndent
folder, because I get the impression that the installed executable does know how to find the library files. However, it might work differently and perl
might 'compile' everything it needs into a single executable which leaves defaultSettings.yaml
dangling outside.
@cmhughes I feel that you might have defaultSettings.yaml
for pedagogical reasons, and that indeed completely makes sense. However, programatically you could consider integrating all default settings in the perl
library, avoiding all similar issues in the future and simplifying user-install quite a bit, and use some command-line option to generate a defaultSettings.yaml
(that you could then keep up-to-date in the repo using e.g. a GitHub Action).
Let's assume that defaultSettings.yaml
must exist for the time being, I don't want us to get side-tracked on that point! :) It has existed pretty much since the beginning, and is a vital way for users to interact with the script.
I completely agree the not side tracking, but I could not resist ;)
Here is what I've done so far on my Ubuntu machine:
sudo snap install pre-commit --classic
mkdir test
cd test
git init
Then I've added myfile.tex
to the repository and also .pre-commit-config.yaml
as follows
repos:
- repo: https://github.com/tdegeus/latexindent.pl
rev: 3.13.5
hooks:
- id: latexindent.pl
args: []
Then I've run
pre-commit try-repo https://github.com/tdegeus/latexindent.pl/ latexindent-cpan --verbose --all-files
This gives me
===============================================================================
Using config:
===============================================================================
repos:
- repo: https://github.com/tdegeus/latexindent.pl/
rev: 5258dee72c1f8169243c4edbce67ca8d86a4f819
hooks:
- id: latexindent-cpan
===============================================================================
[INFO] Initializing environment for https://github.com/tdegeus/latexindent.pl/.
An error has occurred: InvalidManifestError:
=====> /tmp/tmpdbvpsgji/repok44nbqni/.pre-commit-hooks.yaml does not exist
Check the log at /home/cmhughes/snap/pre-commit/common/.cache/pre-commit/pre-commit.log
Now I need to dig into the error. The pre-commit.log
is below, for reference.
When you do pre-commit try-repo https://github.com/tdegeus/latexindent.pl/
it will clone the main branch but the .pre-commit-hooks.yaml file is added in the pre-commit branch. You'll need to:
pre-commit try-repo <PATH_TO_THE_LOCAL_CLONE> latexindent-cpan --verbose --all-files
P.S: I think it is better if you install pre-commit with pip instead of snap: python3 -m pip install pre-commit
.
This way you get the latest version of pre-commit (which only works with python 3.6+ and according to the log output you provided, you have a version that works with python3.5 which is definitely old).
Many thanks, that's helpful.
Here's what I've done:
python3 -m pip install pre-commit
I then updated my path
via .bashrc
so that it includes
export PATH=$PATH:/home/cmhughes/.local/bin
I then ran
git clone https://github.com/tdegeus/latexindent.pl/
git checkout --track origin/pre-commit
Then running
pre-commit try-repo /home/cmhughes/projects/tdegues-latexindent/ latexindent-cpan --verbose --all-files
gives
===============================================================================
Using config:
===============================================================================
repos:
- repo: ../../projects/tdegues-latexindent
rev: 149a22c6251db129dbac767efc3d0541b6a8991c
hooks:
- id: latexindent-cpan
===============================================================================
[INFO] Initializing environment for ../../projects/tdegues-latexindent.
[INFO] Installing environment for ../../projects/tdegues-latexindent.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
latexindent.pl...........................................................Failed
- hook id: latexindent-cpan
- duration: 0.38s
- exit code: 2
FATAL Could not open defaultSettings.yaml
I then applied the changes proposed above by @hamdanal to GetYamlSettings.pm
and moved defaultSettings.yaml
into the LatexIndent
folder.
Then running
pre-commit try-repo /home/cmhughes/projects/tdegues-latexindent/ latexindent-cpan --verbose --all-files
gave me:
===============================================================================
Using config:
===============================================================================
repos:
- repo: ../../projects/tdegues-latexindent
rev: 5649ccb8ede106398a169737a82bbddba40443a5
hooks:
- id: latexindent-cpan
===============================================================================
[INFO] Initializing environment for ../../projects/tdegues-latexindent.
[INFO] Installing environment for ../../projects/tdegues-latexindent.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
latexindent.pl...........................................................Failed
- hook id: latexindent-cpan
- duration: 0.26s
- files were modified by this hook
which I think is what I would want to happen, given that the file in the repository had not previously been operated upon by latexindent.pl
.
I need to figure out why defaultSettings.yaml
can't live in its usual place.
Perhaps we should be making use of the --check
feature of latexindent.pl
...? All comments welcome!
So it seems that things work. If you would run pre-commit once more you would find that it “passes” for latexindent. --check
does not have to used, pre-commit takes care of checking if the file was changed or not.
So indeed the defaultSettings.yaml
needs to be fixed. To me, the fix by @hamdanal seems like the fix for the following reason (I'm extrapolating from Python, this might not be how perl actually works):
MakeFile.PL
installs latexindent.pl
in a binary directory associated to perl
(which probably varies depending on platform and installation)
EXE_FILES => ['latexindent.pl'],
while it places the LatexIndent
folder in a library directory known to perl
.
NAME => "LatexIndent",
Then, latexindent.pl
knows how to find its library
use LatexIndent
but when looking for defaultSettings.yaml
it gets lost:
} elsif ( -e "$FindBin::RealBin/LatexIndent/defaultSettings.yaml" ) {
looks in a subfolder of the binary directory, while in fact defaultSettings.yaml
is in the library directory that is almost certainly somewhere else.
The fix by @hamdanal solves this
my $dirName = dirname(__FILE__);
points to the library directory, as that is where GetYamlSettings.pm
is stored.
I'll take the liberty to implement all comments and fixes for a full and complete review
Ready for your review. As far as the implementation is concerned things are good to go I would say. However:
defaultSettings.yaml
still needs to be copied to LatexIndent
. I don't know how you wish to manage that @cmhughes Thanks @hamdanal for the fix!
Thanks, that's great. Thanks also for rebasing this from develop
.
I've done some more testing, and using the following line in Makefile.PL
seems to fix the defaultSettings
issue
EXE_FILES => ['latexindent.pl','defaultSettings.yaml'],
Can you check this at your end, and push a commit to your branch?
Some more questions from me:
in my repository, I created .pre-commit-config.yaml
repos:
- repo: https://github.com/tdegeus/latexindent.pl
rev: 3.13.5
hooks:
- id: latexindent.pl
args: [-l]
and was hoping that it would instruct latexindent.pl
to use the arguments I specified. I checked indent.log
and this was not the case. How do I fix this?
I've been using the command that you provided
pre-commit try-repo /home/cmhughes/projects/tdegues-latexindent/ latexindent --verbose --all-files
but how can I test how this will work when it's live on the repository?
Is it the case that, when working, a user wouldn't be running a command manually, and that it would simply happen?
I think we're getting very close to merging this! Thanks for your work :)
Done I think :
defaultSettings.yaml
are now copied to the bin
like indicated. Personally I think that for the long-term I think that a different solution should be favoured as outlined above. But this a fine work-around.args: []
in the hook. Can you check that it is fixed now?.pre-commit-config.yaml
set-up you simply run pre-commit run --all-files
, this will run all hooks (there could obviously be several) on all relevant files. So you could have a small external repo use the hook and have CI run on it.@cmhughes I finally tested on some remote linux machine I got:
make: *** No rule to make target `defaultSettings.yaml', needed by `blib/script/defaultSettings.yaml'. Stop.
so I'm not sure that your workaround actually works. Could you comment?
Perhaps a better option is available from https://perldoc.perl.org/ExtUtils::MakeMaker
Goal: copy defaultSettings.yaml to the LatexIndent folder.
What I found from searching is exactly your solution. I just don't get why I didn't work for me. Maybe you had a slightly different syntax? Does the current commit work for you?
Minor update: for me extra options work (tested using the latexindent-conda
hook)
The makefile seems to know to copy the LatexIndent folder without explicitly being told to do so, probably because it knows to look for pm files.
If we can find a way to make it do the same for yaml files, that might help.
On Wed, 19 Jan 2022, 11:47 Tom de Geus, @.***> wrote:
Minor update: for me extra options work (tested using the latexindent-conda hook)
— Reply to this email directly, view it on GitHub https://github.com/cmhughes/latexindent.pl/pull/322#issuecomment-1016390062, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQ7CYBJGM6ZWPVXXSGDOFLUW2QF7ANCNFSM5LTEZ3HA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you were mentioned.Message ID: @.***>
I tweaked Makefile.PL
to copy LatexIndent
by doing this:
NAME => "LatexIndent",
Here is what I think happens:
LatexIndent
is installed in a perl library directory, that can then be called from a perl
script using use LatexIndent
.latexindent.pl
is installed in a binary directory, that can because of 1 finds its library routines. This means that placing defaultSettings.yaml
in LatexIndent
(or placing a copy there) would solve everything.
So now I think that that is what happens, and that that is solution; I'm not a perl
user and even further from being an expert. What could also happen:
Makefile.PL
and from a perl
session calling use LatexIndent
..pm
files are installed in the library directory. But earlier remarks by @hamdanal seem to exclude this. This means that placing defaultSettings.yaml in LatexIndent would solve everything.
I agree. This would require changing any occurrence of .../defaultSettings.yaml
to .../LatexIndent/defaultSettings.yaml
. I am thinking in:
LatexIndent/GetYamlSettings.pm
file...(or placing a copy there)
I think this should be avoided as it becomes easy to update one file and not the other. It would be an unnecessary added burden on the maintainer to always keep the two copies in sync.
If defaultSettings.yaml is to be moved, please let me do it on develop. There are a lot of consequences to this :)
I'm going to continue to try to avoid having to do this, updates to follow when I have them.
On Wed, 19 Jan 2022, 15:54 hamdanal, @.***> wrote:
This means that placing defaultSettings.yaml in LatexIndent would solve everything.
I agree. This would require changing any occurrence of .../defaultSettings.yaml to .../LatexIndent/defaultSettings.yaml. I am thinking in:
- the builders (in github actions and elsewhere);
- the LatexIndent/GetYamlSettings.pm file
...(or placing a copy there)
I think this should be avoided as it becomes easy to update one file and not the other. It would be an unnecessary added burden on the maintainer to always keep the two copies in sync.
— Reply to this email directly, view it on GitHub https://github.com/cmhughes/latexindent.pl/pull/322#issuecomment-1016607558, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQ7CYCGWEZVTJLCKJTQNGTUW3NFHANCNFSM5LTEZ3HA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you were mentioned.Message ID: @.***>
@cmhughes I completely understand. Let us, therefore, circle back to what you commented yesterday
I've done some more testing, and using the following line in Makefile.PL seems to fix the defaultSettings issue
EXE_FILES => ['latexindent.pl','defaultSettings.yaml'],
So I thought: that's odd, why doesn't it work for me?
In the documentation it says:
EXE_FILES Ref to array of executable files. The files will be copied to the INST_SCRIPT directory. Make realclean will delete them from there again.
If your executables start with something like
#!perl
or#!/usr/bin/perl
MakeMaker will change this to the path of the perl 'Makefile.PL' was invoked with so the programs will be sure to run properly even if perl is not in/usr/bin/perl
.
So what happened it seems is that perl
was trying to interpret the yaml
comment on the first line as a shebang.
Adding one white line solves things, and makes everything good to go.
I must say that relying on the first line not being a yaml
comment is a bit bug prone, so at some point I would suggest a permanent solution (e.g. along the lines suggested above)
P.S. speaking of pre-commit: it provides great tools to keep source files clean of trailing spaces. Some editors, like mine, are set to remove them, leading to noise in PRs. (Sorry for staging the entire file ;))
My previous work around is feeling increasingly like it is not a solution.
Sometimes life is surprisingly simple. Please checkout the latest commit !!
That's great, nicely done. I think we're getting closer to merging this.
The following test worked as expected:
pre-commit try-repo /home/cmhughes/projects/tdegues-latexindent/ latexindent --verbose --all-files
I wanted to try testing customisation of .pre-commit-config.yaml
arguments, so I followed https://github.com/pre-commit/pre-commit/issues/850 which allowed me to create .pre-commit-config.yaml
as folllows
repos:
- repo: ../../projects/tdegues-latexindent
rev: 730d449e6506b59f12105a17184bfaa3c1e8034a
hooks:
- id: latexindent
args: [-l,-m]
I could then run the following command successfully:
pre-commit run --all-files
I verified it by checking indent.log
.pre-commit-hooks.yaml
we have the line entry: latexindent.pl -w -s
can this line be customized in a user file .pre-commit-config.yaml
? If not, then I would like to have all of the arguments removed, so that the entry
line would simply be entry: latexindent.pl
so that users can add them as they see fit in their own configuration files.cls
and sty
files in my repository, how do I customise my .pre-commit-config.yaml
to accomodate these?environment.yml
: does this need to live in the main repository? Can it go in a directory, such as conda
? Great that you can confirm that it works. I let you decide how/if you CI on this.
That should do it @cmhughes
This is great, thank you!
I'm really grateful for your contribution, and for your time in answering my questions.
I've completed testing, and I'm very happy to merge this now. The test-cases.sh
go through as expected, and the commands pre-commit
commands detailed above, both for cpan
and conda
work as expected.
I've put some details below, purely for my own reference, should I need to return to this.
cd /home/projects
git clone https://github.com/tdegeus/latexindent.pl/
mv latexindent.pl/ tdegues-latexindent/
cd tdegues-latexindent/
git checkout --track origin/pre-commit
cd /home/cmhughes/Desktop/test
pre-commit try-repo /home/cmhughes/projects/tdegues-latexindent/ latexindent --verbose --all-files
this was successful
this seemed difficult with try-repo, so I followed the instructions at https://github.com/pre-commit/pre-commit/issues/850
this allowed me to create .pre-commit-config.yaml
containing the following
repos:
- repo: ../../projects/tdegues-latexindent
rev: 730d449e6506b59f12105a17184bfaa3c1e8034a
hooks:
- id: latexindent
args: [-l,-m]
I could then run
pre-commit run --all-files
and verify, via indent.log
, that the -l and -m switches were passed correctly to latexindent.pl
this was successful
This will be useful. Thank you both very much.
Fixes https://github.com/cmhughes/latexindent.pl/issues/316
MakeFile.PL
way, but the one I wrote does not download dependencies it seems. I kept my attempt for discussion.conda
where everything is working I think. However, I cannot test on my machine until https://github.com/cmhughes/latexindent.pl/issues/303 is fixed. @cmhughes any news on this?