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

Implement TikZ images for problems #1030

Merged
merged 2 commits into from
Dec 28, 2019
Merged

Conversation

drgrice1
Copy link
Member

@@ -128,7 +133,7 @@ $externalPrograms{pngtopnm} = "$netpbm_prefix/pngtopnm";
####################################################
# set timeout time (-t 40 sec) to be less than timeout for problem (usually 60 seconds)
$externalPrograms{checkurl} = "/usr/bin/lwp-request -d -t 40 -mHEAD "; # or "/usr/local/bin/w3c -head "
$externalPrograms{curlCommand} = "/usr/bin/curl";
Copy link
Member

@mgage mgage Dec 25, 2019

Choose a reason for hiding this comment

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

the curlCommand is used in a number of places. It would be better to leave it for now (in addition to the
definition for {curl}

Copy link
Member

@mgage mgage Dec 25, 2019

Choose a reason for hiding this comment

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

/Volumes/WW_test/opt/webwork/webwork2_2018/lib/WeBWorK/PG.pm:211: $envir{externalCurlCommand} = $ce->{externalPrograms}->{curlCommand};
and
/Volumes/WW_test/opt/webwork/pg_2018/lib/PGcore.pm:769: $options->{curlCommand} = WeBWorK::PG::IO::curlCommand(); #FIXME just changed from alternate source

Also curlCommand occurs in several places in PG/IO.pm where it looks like the code is in the midst of a change over from curlCommand to curl. Using curlCommand within IO.pm and askSage() might
be ok -- it makes some of the commands to askSage clearer. I could go either way.

#FIXME change {curlCommand} to {curl}, here, and site.conf 

sub WeBWorK::PG::IO::curlCommand {
	# $CE->{externalPrograms}->{curlCommand};
	$CE->{externalPrograms}->{curl};
}

The other places, outside of those subroutines it seems to me that using {curl} as you have done is clearer. I no longer remember my exact thinking when I was last working on this code. What do you think? should we change all occurrences of curlCommand to curl?

Copy link
Member Author

@drgrice1 drgrice1 Dec 25, 2019

Choose a reason for hiding this comment

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

I could put it back if you think that would be safer. I was just trying to complete what was done in your tikz pull request. It does seem the naming was inconsistent relative to the other external program variables. I had thought I changed all instances of {curl command} to {curl}. Perhaps I missed some.

Copy link
Member

Choose a reason for hiding this comment

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

On closer inspection I think you only missed one. The line
211 in PG.pm . This defines a variable in the PG environment called
externalCurlCommand. I don't see where that is ever used.

I think I'm in favor of commenting out line 211 in PG.pm (see whether that ever causes problems).

I've decided I'm ok with continuing to use curlCommand inside askSage and IO.pm since I believe my idea there was that we might want to add additional switches to the bare curl command and therefore created a front end subroutine to make it easier to modify the bare curl command if we needed to. I'm willing to change my mind on that if you think that is over complicating things.

I was trying to make the assignments in site.conf more uniform -- and assigning {curlCommand} stuck out -- so you were following my initial intentions there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. I did miss line 211 of PG.pm. We can comment that out, or just adjust it to be {curl}, or put everything back the way it was. That isn't part of the tikz image work, and I don't want it to interfere with getting that in. Just let me know what you prefer.

Copy link
Member Author

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

I didn't remove the curl command. I just renamed it (based on your suggestion in the old code), and I have checked that all uses have the new name.

@mgage
Copy link
Member

mgage commented Dec 26, 2019 via email

to $externalPrograms{curl}.  Then comment that out as per the discussion
in PR openwebwork#1030.
@taniwallach
Copy link
Member

A comment about the need to make the change to /etc/ImageMagick-6/policy.xml to use the feature added (to allow use of /usr/bin/convert to process PDF files) should be include in the install instructions and release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants