Skip to content

Conversation

@c42f
Copy link
Member

@c42f c42f commented Nov 1, 2018

Log ids are meant to identify the location of the message in the source code and must be computed at compile time.

For further commentary on this proposed fix see #29355 (comment) and #29355 (comment)

Fixes #28786, fixes #28400 and closes #29355.

@adamslc I incorporated your tests in here (lightly modified)

@oxinabox @iamed2 @samoconnor you'll need this to fix #28400 before using the new logging infrastructure in any serious way for long running tasks. [Yikes, what a horrible bug :-( ]

Luke Adams and others added 3 commits November 1, 2018 09:01
Log ids are meant to identify the location of the message in the source
code and must be computed at compile time.

Fixes #28786, #28400; replaces #29355.
@c42f c42f added logging The logging framework backport pending 1.0 labels Nov 1, 2018
@ararslan ararslan added the bugfix This change fixes an existing bug label Nov 1, 2018
@c42f c42f added the embarrassing-bugfix Whoops! label Nov 1, 2018
@c42f
Copy link
Member Author

c42f commented Nov 1, 2018

There was a couple of win32 CI profile test failures which looked bizarre and unrelated so I restarted that. Here's a separate issue for that: #29880

@adamslc adamslc mentioned this pull request Nov 1, 2018
@KristofferC KristofferC added the priority This should be addressed urgently label Nov 1, 2018
@KristofferC
Copy link
Member

Putting a priority label on this since we want this in 1.0.2 which we want to run PkgEval and release as soon as possible.

@JeffBezanson
Copy link
Member

LGTM.

@fredrikekre fredrikekre merged commit 51683c4 into master Nov 1, 2018
@fredrikekre fredrikekre deleted the cjf/fix-logging-ids branch November 1, 2018 18:15
KristofferC pushed a commit that referenced this pull request Nov 1, 2018
* fix bug and add tests

* Reinstate statically computed log record ids

Log ids are meant to identify the location of the message in the source
code and must be computed at compile time.

fix #28786, fix #28400; closes #29355.

* Clarify documentation regarding log record `id`

(cherry picked from commit 51683c4)
@c42f
Copy link
Member Author

c42f commented Nov 1, 2018

Thanks guys, getting this into 1.0.2 will be great.

@adamslc looks like your patch got mashed together with mine in the merge. Sorry about that :-/

@adamslc
Copy link
Contributor

adamslc commented Nov 1, 2018

No worries. Just glad to have this bug fixed.

KristofferC pushed a commit that referenced this pull request Nov 2, 2018
* fix bug and add tests

* Reinstate statically computed log record ids

Log ids are meant to identify the location of the message in the source
code and must be computed at compile time.

fix #28786, fix #28400; closes #29355.

* Clarify documentation regarding log record `id`

(cherry picked from commit 51683c4)
odow added a commit to jump-dev/JuMP.jl that referenced this pull request Dec 30, 2018
odow added a commit to jump-dev/JuMP.jl that referenced this pull request Dec 30, 2018
* Remove outdated comment

We retain `copy_names = true` for explicitness.

* Remove outdated comment.

This was fixed by JuliaLang/julia#29878
KristofferC pushed a commit that referenced this pull request Feb 11, 2019
* fix bug and add tests

* Reinstate statically computed log record ids

Log ids are meant to identify the location of the message in the source
code and must be computed at compile time.

fix #28786, fix #28400; closes #29355.

* Clarify documentation regarding log record `id`

(cherry picked from commit 51683c4)
KristofferC pushed a commit that referenced this pull request Feb 20, 2020
* fix bug and add tests

* Reinstate statically computed log record ids

Log ids are meant to identify the location of the message in the source
code and must be computed at compile time.

fix #28786, fix #28400; closes #29355.

* Clarify documentation regarding log record `id`

(cherry picked from commit 51683c4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This change fixes an existing bug embarrassing-bugfix Whoops! logging The logging framework priority This should be addressed urgently

Projects

None yet

Development

Successfully merging this pull request may close these issues.

in warn macro the maxlog keyword is not effective log_record_id runtime increases quadratically with duplicate logs

6 participants