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

missing includer issue #122

Closed
wants to merge 14 commits into from
Closed

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Jul 2, 2020

EDIT: fixed in aminya/CompileBot.jl#5

To fix missing includer issue in JuliaPlots/Plots.jl#2838

⚠️ emergency patch

  • Adds gitattributes to prevent line ending changing during uploads from different operating systems
  • removes previous bash script that did a similar thing
  • throws an error for anyone who uses the bash script (there is no other way to let them know 😞)

Let's merge this before people run into the same issue.

@aminya
Copy link
Contributor Author

aminya commented Jul 2, 2020

@timholy "Lost of methods" tests fail in the nightly. Has anything changed in nightly?

@aminya
Copy link
Contributor Author

aminya commented Jul 2, 2020

I wonder if f50b27a actually fix the issue. The bash script for discarding whitespace changes is very hacky and unreliable. I prefer to write a Julia script to achieve the goal.

Running it on https://github.com/JuliaPlots/Plots.jl/suites/862271274/artifacts/9944298, solves the issue, but we might want to test it on more packages.

This is a patch that we cannot send to our users unless we find all the repositories that use SnoopCompile.yml. 😞 This line of code was added as a workaround for actions/upload-artifact#75. But now it is a wrecker for the packages that rely on precompilation so much that without it they break (e.g Plots).

We should take #99 serious now. Probably adding a warning to snoop_bot and snoop_bench to update their workflow files, and use GitHubActions.jl to bring it to their notice:

using Logging: global_logger
using GitHubActions: GitHubActionsLogger
get(ENV, "GITHUB_ACTIONS", "false") == "true" && global_logger(GitHubActionsLogger())

@aminya aminya changed the title Plots issues? add_includer issues Jul 2, 2020
@aminya aminya changed the title add_includer issues missing includer issue Jul 2, 2020
aminya added 2 commits July 2, 2020 06:13
This reverts commit 113c28f.

GitHubActions seem to fail building!
@aminya aminya force-pushed the missing_includer branch from a2b74eb to b73fa61 Compare July 2, 2020 11:42
@aminya aminya force-pushed the missing_includer branch from 9239118 to eceafb3 Compare July 2, 2020 12:11
@aminya aminya force-pushed the missing_includer branch from eceafb3 to e77c8dc Compare July 2, 2020 12:25
@aminya
Copy link
Contributor Author

aminya commented Jul 6, 2020

@timholy Could you take a look at this?

@timholy
Copy link
Owner

timholy commented Jul 8, 2020

Hmm, messing with a repo's git attributes seems beyond the scope of SnoopCompile, and it seems like material better placed in PkgTemplates than here. Is there a more minimal fix possible?

@aminya
Copy link
Contributor Author

aminya commented Jul 8, 2020

Hmm, messing with a repo's git attributes seems beyond the scope of SnoopCompile, and it seems like material better placed in PkgTemplates than here. Is there a more minimal fix possible?

Yes, it should be in PkgTemplates too, but specifically for this case, we need some sort of EOL managing. One way I can think of is to have a SnoopCompileBot.post_generation function that runs in the Create_PR step, and we can do some post generation checks or fixes in it. Such as fixing line-ending issues, checking the existance of includer, or any compatibility things that may arise later.

@timholy
Copy link
Owner

timholy commented Jul 8, 2020

I'd like this to be as small & non-intrusive a fix as possible for this bug, and then let's split the bot out into a separate package and you can do more heroic repo-surgery there.

@timholy
Copy link
Owner

timholy commented Jul 8, 2020

Or alternatively, if it really is hard to fix this without something big (notifying all existing users, whatever), then let's just do the split now and you can fix in 2.0?

@aminya
Copy link
Contributor Author

aminya commented Jul 10, 2020

As I said already, I am OK with splitting the bot into a separate repository. We can do it at any time.

For later fixes, I want to add that post_generation function to refactor the Create_PR step in SnoopCompile.yml.

Currently, I have to error to inform the users about the faulty bash script. That only affects the users of multios/multiversion configuration. We should fix this now before switching to 2. We should have a working 1.x release.

@timholy
Copy link
Owner

timholy commented Aug 3, 2020

As mentioned above, I don't plan to merge this before we switch to 2.0. Changing the git settings on the repo by default is beyond the scope of this package. I recommend creating the new bot repo package soon.

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