knitr 1.45 seems to have broken the collectcode chunk option

remlapmot commented 1 year ago

Hi Doug

knitr 1.45 was released on 30^th October.

I think I notice that it has broken the collectcode chunk option.

I have put a demo at https://github.com/remlapmot/collectcode-error (the error is still present under Yihui's latest version on GitHub, but I didn't report this in the knitr repo yet).

Correct output under knitr 1.44 https://htmlpreview.github.io/?https://github.com/remlapmot/collectcode-error/main/test-knitr-1.44.html
Incorrect output under knitr 1.45 https://htmlpreview.github.io/?https://github.com/remlapmot/collectcode-error/main/test-knitr-1.45.html
Incorrect output under knitr devel https://htmlpreview.github.io/?https://github.com/remlapmot/collectcode-error/main/test-knitr-devel.html

I haven't traced which change in knitr 1.45 causes the error but I know the differences between the knitr 1.44 and 1.45 can be viewed at

https://github.com/yihui/knitr/compare/v1.44...v1.45

If we can work out which was the breaking change maybe we should report to Yihui?

All the best Tom

Hemken commented 1 year ago

I’m out of town, traveling this week.

You could put a query at the beginning of the second Stata code chunk to see if the profile.do file exists at that point. The error message suggests that auto.dta wasn’t even loaded.

From: Tom Palmer @.> Sent: Monday, November 6, 2023 8:49:00 AM To: Hemken/Statamarkdown @.> Cc: Subscribed @.***> Subject: [Hemken/Statamarkdown] knitr 1.45 seems to have broken the collectcode chunk option (Issue #38)

Hi Doug

knitr 1.45 was released on 30th October.

I think I notice that it has broken the collectcode chunk option.

I have put a demo at https://github.com/remlapmot/collectcode-error (the error is still present under Yihui's latest version on GitHub, but I didn't report this in the knitr repo yet).

Correct output under knitr 1.44 https://htmlpreview.github.io/?https://github.com/remlapmot/collectcode-error/main/test-knitr-1.44.html
Incorrect output under knitr 1.45 https://htmlpreview.github.io/?https://github.com/remlapmot/collectcode-error/main/test-knitr-1.45.html
Incorrect output under knitr devel https://htmlpreview.github.io/?https://github.com/remlapmot/collectcode-error/main/test-knitr-devel.html

I haven't traced which change in knitr 1.45 causes the error but I know the differences between the knitr 1.44 and 1.45 can be viewed at

@.***https://github.com/yihui/knitr/compare/v1.44...v1.45

If we can work out which was the breaking change maybe we should report to Yihui?

All the best Tom

— Reply to this email directly, view it on GitHubhttps://github.com/Hemken/Statamarkdown/issues/38, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACYBME6CNHT2PPI3477EHV3YDEBGZAVCNFSM6AAAAAA67XJTHSVHI2DSMVQWIX3LMV43ASLTON2WKOZRHE3TSNBXGYYTGNI. You are receiving this because you are subscribed to this thread.Message ID: @.***>

remlapmot commented 1 year ago

Interesting, under 1.45 profile.do isn't present after the first chunk. I'm not sure if it survives the first chunk.

remlapmot commented 1 year ago

I have run using knitr under each commit into 1.45 and found that the first time the error occurs is at this commit

https://github.com/yihui/knitr/commit/87254086b81c12451ccbfb36c36653e259a1c54d

(which you can install with)

remotes::install_github("yihui/knitr@8725408")

Hemken commented 1 year ago

So I doubt it’s the concordance.R. So which of these is it: output.R, parser.R, utils.R?

-- Doug Hemken SSCC statistical consultant 4226i Social Science Bldg.

@.**@.> 608-262-4327

From: Tom Palmer @.> Sent: Tuesday, November 7, 2023 4:19 AM To: Hemken/Statamarkdown @.> Cc: Doug Hemken @.>; Comment @.> Subject: Re: [Hemken/Statamarkdown] knitr 1.45 seems to have broken the collectcode chunk option (Issue #38)

I have run using knitr under each commit into 1.45 and found that the first time the error occurs is at this commit

@.***https://github.com/yihui/knitr/commit/87254086b81c12451ccbfb36c36653e259a1c54d

— Reply to this email directly, view it on GitHubhttps://github.com/Hemken/Statamarkdown/issues/38#issuecomment-1798214228, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACYBMEYKLGZ3IGY436QUQPLYDIDKHAVCNFSM6AAAAAA67XJTHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJYGIYTIMRSHA. You are receiving this because you commented.Message ID: @.***>

remlapmot commented 1 year ago

I have tested by making the changes in the commit file by file and find that it's the changes in output.R - but I don't know which of the changes in that file causes the problem.

Hemken commented 1 year ago

Great! I’ve just been setting up a test environment for myself, and that saves me several steps!

-- Doug Hemken SSCC statistical consultant 4226i Social Science Bldg.

@.**@.> 608-262-4327

From: Tom Palmer @.> Sent: Wednesday, November 8, 2023 12:43 PM To: Hemken/Statamarkdown @.> Cc: Doug Hemken @.>; Comment @.> Subject: Re: [Hemken/Statamarkdown] knitr 1.45 seems to have broken the collectcode chunk option (Issue #38)

I have tested by making the changes in the commit file by file and find that it's the changes in output.R - but I don't know which of the changes in that file causes the problem.

— Reply to this email directly, view it on GitHubhttps://github.com/Hemken/Statamarkdown/issues/38#issuecomment-1802449955, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACYBME66HDWYC6IJQ42KGDTYDPHDLAVCNFSM6AAAAAA67XJTHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBSGQ2DSOJVGU. You are receiving this because you commented.Message ID: @.***>

Hemken commented 1 year ago

Somehow those changes, which appear to be in a error handler (?) prevent the profile.do from being written (or from being written in the correct location?)

-- Doug Hemken Statistical consultant Social Science Computing Cooperative Univ. of Wisc. – Madison

4226I Social Science Bldg.

@.*** 608-262-4327

To make a consulting appointment send me an email, or use the on-line scheduler: https://doodle.com/mm/doughemken/book-a-time

From: Tom Palmer @.> Sent: Wednesday, November 8, 2023 12:43 PM To: Hemken/Statamarkdown @.> Cc: Doug Hemken @.>; Comment @.> Subject: Re: [Hemken/Statamarkdown] knitr 1.45 seems to have broken the collectcode chunk option (Issue #38)

I have tested by making the changes in the commit file by file and find that it's the changes in output.R - but I don't know which of the changes in that file causes the problem.

— Reply to this email directly, view it on GitHubhttps://github.com/Hemken/Statamarkdown/issues/38#issuecomment-1802449955, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACYBME66HDWYC6IJQ42KGDTYDPHDLAVCNFSM6AAAAAA67XJTHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBSGQ2DSOJVGU. You are receiving this because you commented.Message ID: @.***>

remlapmot commented 1 year ago

I agree. And I tried an example with an existing profile.do and find that it's not appended with the collectcode commands as it usually would be.

remlapmot commented 1 year ago

My only idea at the moment is whether the number in the sys.frame() call might need to be amended - maybe the new code has caused the numbers to change?

[edited line 44 to line 40] https://github.com/Hemken/Statamarkdown/blob/1e52b2370b623b7fdf20d87cb1068f9fa34cd216/R/stata_collectcode.r#L40

remlapmot commented 1 year ago

I amended that -9 to a -10 and collectcode seems to work again.

So we could set it conditionally on the knitr version number, i.e.,

        if (packageVersion('knitr') < '1.45') {
            do.call("on.exit",
                list(quote(unlink("profile.do")), add=TRUE),
                envir = sys.frame(-9))
        }
        if (packageVersion('knitr') >= '1.45') {
            do.call("on.exit",
                list(quote(unlink("profile.do")), add=TRUE),
                envir = sys.frame(-10))
        }

(Also it's not clear to me whether we additionally need to amend the sys.frame(-9) in the if statement below this; currently on line 44, for knitr 1.45)

Hemken commented 1 year ago

Brilliant! (This is one of the two most obscure-to-me parts of the code I wrote!)

-- Doug Hemken SSCC statistical consultant 4226i Social Science Bldg.

@.**@.> 608-262-4327

From: Tom Palmer @.> Sent: Thursday, November 9, 2023 9:26 AM To: Hemken/Statamarkdown @.> Cc: Doug Hemken @.>; Comment @.> Subject: Re: [Hemken/Statamarkdown] knitr 1.45 seems to have broken the collectcode chunk option (Issue #38)

I amended that -9 to a -10 and collectcode seems to work again.

So we could set it conditionally on the knitr version number, i.e.,

    if (packageVersion('knitr') < '1.45') {

        do.call("on.exit",

            list(quote(unlink("profile.do")), add=TRUE),

            envir = sys.frame(-9))

    }

    if (packageVersion('knitr') >= '1.45') {

        do.call("on.exit",

            list(quote(unlink("profile.do")), add=TRUE),

            envir = sys.frame(-10))

    }

(Also it's not clear to me whether we additionally need to amend the sys.frame(-9) in the if statement below this; currently on line 44, fot knitr 1.45)

— Reply to this email directly, view it on GitHubhttps://github.com/Hemken/Statamarkdown/issues/38#issuecomment-1804042722, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACYBMEYJ5S32CKLG5PPEGM3YDTYXLAVCNFSM6AAAAAA67XJTHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBUGA2DENZSGI. You are receiving this because you commented.Message ID: @.***>

Hemken commented 1 year ago

Yes, those would both need to be changed.

Thanks for the code, I’ll implement it.

-- Doug Hemken SSCC statistical consultant 4226i Social Science Bldg.

@.**@.> 608-262-4327

From: Tom Palmer @.> Sent: Thursday, November 9, 2023 9:26 AM To: Hemken/Statamarkdown @.> Cc: Doug Hemken @.>; Comment @.> Subject: Re: [Hemken/Statamarkdown] knitr 1.45 seems to have broken the collectcode chunk option (Issue #38)

I amended that -9 to a -10 and collectcode seems to work again.

So we could set it conditionally on the knitr version number, i.e.,

    if (packageVersion('knitr') < '1.45') {

        do.call("on.exit",

            list(quote(unlink("profile.do")), add=TRUE),

            envir = sys.frame(-9))

    }

    if (packageVersion('knitr') >= '1.45') {

        do.call("on.exit",

            list(quote(unlink("profile.do")), add=TRUE),

            envir = sys.frame(-10))

    }

(Also it's not clear to me whether we additionally need to amend the sys.frame(-9) in the if statement below this; currently on line 44, fot knitr 1.45)

— Reply to this email directly, view it on GitHubhttps://github.com/Hemken/Statamarkdown/issues/38#issuecomment-1804042722, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACYBMEYJ5S32CKLG5PPEGM3YDTYXLAVCNFSM6AAAAAA67XJTHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBUGA2DENZSGI. You are receiving this because you commented.Message ID: @.***>

remlapmot commented 1 year ago

Great, thanks Doug.

It must be that the added handle_error() function call (or maybe the knit_concord$get('block')) bumps the sys.frame() number.

Do you think it is worth informing Yihui about this, so that he's aware of the issue in case any other knitr language engines have a similar issue?

remlapmot commented 1 year ago

Nb. I think I should have put the utils:: package qualifier on the packageVersion() calls otherwise you'll get an R CMD check note, i.e., utils::packageVersion('knitr')

Hemken commented 1 year ago

It is my impression that this is becoming standard style across a number of programming languages.

-- Doug Hemken SSCC statistical consultant 4226i Social Science Bldg.

@.**@.> 608-262-4327

From: Tom Palmer @.> Sent: Thursday, November 9, 2023 10:23 AM To: Hemken/Statamarkdown @.> Cc: Doug Hemken @.>; Comment @.> Subject: Re: [Hemken/Statamarkdown] knitr 1.45 seems to have broken the collectcode chunk option (Issue #38)

Nb. I think I should have put the utils:: package qualifier on the packageVersion() calls otherwise you'll get an R CMD check note, i.e., utils::packageVersion('knitr')

— Reply to this email directly, view it on GitHubhttps://github.com/Hemken/Statamarkdown/issues/38#issuecomment-1804146849, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACYBME4I7GPSRXENFWV3YOTYDT7O5AVCNFSM6AAAAAA67XJTHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBUGE2DMOBUHE. You are receiving this because you commented.Message ID: @.***>

Hemken commented 1 year ago

It will affect SASmarkdown, too. But I’m not aware of many other language engines that work this way.

-- Doug Hemken SSCC statistical consultant 4226i Social Science Bldg.

@.**@.> 608-262-4327

From: Tom Palmer @.> Sent: Thursday, November 9, 2023 10:17 AM To: Hemken/Statamarkdown @.> Cc: Doug Hemken @.>; Comment @.> Subject: Re: [Hemken/Statamarkdown] knitr 1.45 seems to have broken the collectcode chunk option (Issue #38)

Great, thanks Doug.

It must be that the added handle_error() function call (or maybe the knit_concord$get('block')) bumps the sys.frame() number.

Do you think it is worth informing Yihui about this, so that he's aware of the issue in case any other knitr language engines have a similar issue?

— Reply to this email directly, view it on GitHubhttps://github.com/Hemken/Statamarkdown/issues/38#issuecomment-1804135675, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACYBME4D3UMYJTOCHSLO7W3YDT6W5AVCNFSM6AAAAAA67XJTHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBUGEZTKNRXGU. You are receiving this because you commented.Message ID: @.***>

remlapmot commented 1 year ago

Ok, that's fine, I won't file an issue in the knitr repo.

Hemken commented 1 year ago

Just pushed a fix to Github. Not sure when exactly, but I’ll run my usual battery of tests (both Windows and Linux) and then submit to CRAN.

-- Doug Hemken SSCC statistical consultant 4226i Social Science Bldg.

@.**@.> 608-262-4327

From: Tom Palmer @.> Sent: Friday, November 10, 2023 7:44 AM To: Hemken/Statamarkdown @.> Cc: Doug Hemken @.>; Comment @.> Subject: Re: [Hemken/Statamarkdown] knitr 1.45 seems to have broken the collectcode chunk option (Issue #38)

Ok, that's fine, I won't file an issue in the knitr repo.

— Reply to this email directly, view it on GitHubhttps://github.com/Hemken/Statamarkdown/issues/38#issuecomment-1805755840, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACYBMEYJU3CXQ7RAGGLMDHDYDYVQLAVCNFSM6AAAAAA67XJTHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBVG42TKOBUGA. You are receiving this because you commented.Message ID: @.***>

remlapmot commented 1 year ago

That's great Doug

I have run my little test using your new version and knitr versions 1.43, 1.44, 1.45, and the latest knitr on GitHub and everything seems to be working well as before.

I'll try and keep it in my mind to run a test when I see new versions of knitr released.

Hemken commented 11 months ago

Fixed with Release 0.9.2

Hemken / Statamarkdown

knitr 1.45 seems to have broken the collectcode chunk option #38