joyfullservice / msaccess-vcs-addin

Synchronize your Access Forms, Macros, Modules, Queries, Reports, and more with a version control system.
Other
203 stars 40 forks source link

Error on Build from Source; automation error #197

Closed hecon5 closed 3 years ago

hecon5 commented 3 years ago

I've been getting the following error lately; any ideas?

ERROR: Importing VBE Project Source: clsDbVbeProject.Import Error -1027541024: Automation error [1]

joyfullservice commented 3 years ago

You might try stepping through the import code... I know there were some interesting things with setting the help context ID. (See additional comments in the source code.)

hecon5 commented 3 years ago

image

Occurs here; forgot to upload :/

hecon5 commented 3 years ago

This just started happening; I'm not sure why as that line hasn't changed in 10 months.

It's also interesting, because the error number is different every time (almost as if it was random);

joyfullservice commented 3 years ago

I have made some recent improvements to the error handling, which might be why you are seeing the error now. I just pushed a commit that may help resolve, or at least further pinpoint the issue. You might also double-check the source file to make sure it isn't trying to use any strange Unicode characters in the help context ID or help file name.

hecon5 commented 3 years ago

I'll run them quick like. Could it have anything to do with having a helpcontextID of zero, and a helpfile (that I didn't set!) of a value?

    "HelpFile": "109753687",
    "HelpContextId": 0,
hecon5 commented 3 years ago

Well, this is interesting. If I STEP through the import, it works fine; but when I just let it run through, it does not.

A9G-Data-Droid commented 3 years ago

Maybe we could add some sanity check to these properties?

For example HelpFile must be a valid path URI and HelpContextId must be numeric.

hecon5 commented 3 years ago

Yes; if not, then just discard the values.

hecon5 commented 3 years ago

@joyfullservice, @A9G-Data-Droid, have some moments to check into this: what would valid values for these look like? I've never used these (and don't intend to in the near term), but I'd really like the error to go away.

joyfullservice commented 3 years ago

After some research on the HelpContextID, it seems pretty conclusive that this is a Long.

On the help file name, it looks like this should be either a *.hlp or *.chm file. From a validation standpoint, I am thinking we should throw a warning on both export and import if this does not appear to be a valid file extension or an existing file. While I am at it, I will go ahead and add support for relative paths.

I do not think we should change or discard the value, since that decision should be left to the developer. The add-in should use the values provided, not attempt to fix or change them. (The developer can remove invalid information in either the source file or the VBA object.)

hecon5 commented 3 years ago

Actually; I don't think the dev can discard these at-will; when I clear them, they still seem to export (and subsequently rebuild on import). Even if the HelpFile is set to "", it still seems to re-import and reset to some random value at random (at least in my instance).

joyfullservice commented 3 years ago

@hecon5 - Are you able to upload a minimal sample database that reproduces the error? That would help in testing...

hecon5 commented 3 years ago

Actually, it does it with this one every so often. When I commit, I just discard that bit, and ignore it; usually imports fine, or if it adds the helpfile in, it doesn't affect me.

hecon5 commented 3 years ago

@A9G-Data-Droid's comment : https://github.com/joyfullservice/msaccess-vcs-integration/pull/187/files#r588449361 shows this happening.

joyfullservice commented 3 years ago

Let me know how it works after 24025a7. I have added some validation to make sure the values we are attempting to use are actually valid. (I.e. an actual *.hlp or *.chm file, not just gibberish for the help file path.) If the value is clearly not legitimate, it is discarded and a warning logged.

hecon5 commented 3 years ago

Still doing it... image

hecon5 commented 3 years ago

image]

Screenshot of the project properties after building from latest dev.

hecon5 commented 3 years ago

Steps: Build from source dev. After successful build, close file (This unloads the addin) reopen, install, this closes file. reopen, open addin>export all source (check full export) : this should use the new version 3.3.21) pop open the repository, notice random value in helpfile setting export.

A9G-Data-Droid commented 3 years ago

Same, it's because the code was added to import and not export. Needs to happen the same on both steps. We can reuse the methods.

A9G-Data-Droid commented 3 years ago

Ok, I refactored and it is working on export now.

It removes the junk:

-    "HelpFile": "100746350",
+    "HelpFile": "",

and throws the warning:

WARNING: '100746350' is not a valid help file name. (Expecting *.hlp or *.chm) Source: clsDbVbeProject.ValidHelpFile

joyfullservice commented 3 years ago

Steps: Build from source dev. After successful build, close file (This unloads the addin) reopen, install, this closes file. reopen, open addin>export all source (check full export) : this should use the new version 3.3.21) pop open the repository, notice random value in helpfile setting export.

I am not able to reproduce the issue using these steps... Building from dev, installing, closing, opening, exporting, etc... I am not seeing any issues with the exported values...

A9G-Data-Droid commented 3 years ago

@joyfullservice The issue happens on Export. I get random numbers on export, every time. So if you are only checking on Import then the source files will still contain the random numbers. I would rather not have the junk in my source files at all. See my PR.

hecon5 commented 3 years ago

Ditto.

joyfullservice commented 3 years ago

@joyfullservice The issue happens on Export. I get random numbers on export, every time. So if you are only checking on Import then the source files will still contain the random numbers. I would rather not have the junk in my source files at all. See my PR.

Absolutely! I just wasn't seeing the junk on export either. But it could be something different with the Access versions, or other factors, so I am happy to validate the data on export as well. Thanks for the PR! I will just make a couple tweaks to use the more advanced error handling. (See explanation below)

You will notice that in this project I am transitioning to the more extended (seemingly redundant) directives like the following: If DebugMode Then On Error Resume Next Else On Error Resume Next

The reason I am doing this is because the On Error directive instantly clears any existing VBA errors. This can easily lead to unknown bugs that are silently ignored when switching between different error directives. The DebugMode function also checks for any existing errors before the next On Error directive, allowing the developer to investigate and resolve these otherwise hidden errors. It also implements the new option to break on VBA errors in the add-in, which helps us debug errors.

hecon5 commented 3 years ago

The DebugMode function also checks for any existing errors before the next On Error directive,

Aha! I wondered about that; that makes sense!

A9G-Data-Droid commented 3 years ago

@joyfullservice so that's known as "Side effect code smell"

The important lesson from that article is known as Curly's Law: Do One Thing

Other developers, like me, are likely to destroy this code, like I did, because it doesn't do what it says. What it says is that it's not needed at all. If you wanted to explicitly check for errors there you would use the method you made for that purpose CatchAny. Then it would be clear to the reader that you are checking for errors.

joyfullservice commented 3 years ago

Yes, I agree that it is not ideal. It's a tough balance between readability, simplicity, and OOP best practices while working within the limitations of VBA. Perhaps I should add some notes on the contributing page to a few nuances with this project that are going to be different from most other VBA projects. (Such as error handling)

joyfullservice commented 3 years ago

@hecon5, @A9G-Data-Droid - Can you verify that the problem has been resolved on the dev branch? I merged the PR with some additional changes and refactoring.

A9G-Data-Droid commented 3 years ago

Testing DB and main project both show this version is good. I think this issue is closed. @hecon5 ? You're the OP, are you good to close?

hecon5 commented 3 years ago

Yep, looks like! Thanks all!

A9G-Data-Droid commented 3 years ago

@hecon5 I made a SO question about this a while ago and it hasn't gotten much attention. You are the only other person I know who has reproduced what I am talking about: https://stackoverflow.com/questions/62095622/why-does-access-vba-project-help-file-name-contain-random-numbers

hecon5 commented 3 years ago

And yet, I still am as stumped as you!

joyfullservice commented 3 years ago

I have seen this in some of my database projects (in Access 2010) but I haven't been able to reproduce it. I just tried creating a blank database, exporting, building from source, running VBA code, etc...

Are there some minimal steps you can outline that consistently create the help file number in a new database?

A9G-Data-Droid commented 3 years ago

@joyfullservice I'm not sure when it began. The project where it happens is my big one. It's got a lot going on. If I look at the history of my project.json I can see that the issue is present 11 months ago. Was it an XML file before that?

hecon5 commented 3 years ago

I notice it now whenever I create any database; I don't know when it started, either.

I also run RD; @A9G-Data-Droid, do you? I wonder if that may be related?

hecon5 commented 3 years ago

Actually, some more research, I think it might be RD's fault: https://github.com/rubberduck-vba/Rubberduck/issues/3355

This seems to imply that RD is storing the project ID there; and because (for whatever reason) VBA's project in Access isn't persistent (same reason you can't sign an Access VBA project like you can with Excel or Word), it randomly changes.

joyfullservice commented 3 years ago

Wow. Nice find! I don't really like the idea of storing a 3rd party identifier in a legitimate field that was designed for a totally different purpose. It seems like a custom property would be a much better solution in Access, but I understand that it wouldn't be as universal across other VBA programs as the VBA project help file property.

Does this even have any benefit to RD if the value changes randomly? If it's not providing anything helpful to RD, I would suggest we just leave the code as is, where it strips out the invalid value on export. Hopefully RD will eventually use another property to store the ID as using the help file field really creates a lot of confusion, in my opinion.

I know that RD is fairly widely used, especially in the circles that would use this add-in, so I am not opposed to adding some compatibility if the ID is truly helpful to the RD system. What do you all think?

hecon5 commented 3 years ago

To me, it's a random value. It's semi-persistent in Excel (the few I've checked), where it could be used to store a persisted "compiled" file check (speeds up loading/parsing), but since Access doesn't do that, I think it's useless.

Options as I see them in order of least to most work:

Since the as-is condition doesn't error out, I am able to just not commit/revert those lines for the bit, so I'm ok without it, but I'm good with the remove option, much like we get rid of GUIDs: they're code noise.

hecon5 commented 3 years ago

Can reopen this, or perhaps making a new issue would be more accurate since technically the original issue (compile/runtime error) is fixed, and the follow-on relates to the resultant code noise?

joyfullservice commented 3 years ago

I am proposing that we add a new option called PreserveRubberDuckID with a default value of false. No UI option, just something that can be manually set in the options file. The code will then check any value in the HelpFile and ignore it if it is just a number, and the option has not been set to true.

I really can't think of a case where a person would want to preserve this value, but since we are discarding it, I think it is helpful to include the option so the user at least has a clue why we are doing it. (This would be consistent with options like StripPublishOption)

I think I can put that together fairly quickly here...

hecon5 commented 3 years ago

I'd suggest instead calling it PreserveHelpFileID, the only reason being it pertains to what is being discarded/saved, so users wouldn't have to know the back story for that value, just that it's the helpfile setting.

joyfullservice commented 3 years ago

Sure. The difference is that some users may actually use a help file. We are not discarding the value unless it appears to be a RubberDuck Identifier. We do preserve the HelpFile, we just don't export other extraneous values that might have gotten put into that field by a 3rd party add-in that has nothing to do with a help file. 😄

hecon5 commented 3 years ago

Fair point; I was thinking something more sledge hammer-esqe, in that it just dumps it and moves on, but I like your nuanced solution :)

joyfullservice commented 3 years ago

I think this should be resolved now. Let me know if you have any issues!

A9G-Data-Droid commented 3 years ago

From Mathieu Guindon:

wiping the project ID is safe as long as RD doesn't re-parse afterwards... it'll think there's a new project loaded and will double-up all the declarations and references (or if it hasn't initially parsed) it'll cause mayhem if it's cleared between any two parse runs

A9G-Data-Droid commented 3 years ago

And the solution given:

if there's a legit path in that property, we leave it alone and treat it as the ID IIRC

So if we make a dummy help file that will serve as the ProjectID for rubberduck.

hecon5 commented 3 years ago

I'm irritated that the solution is to create a fake file and add data that wasn't put there by a dev/intentionally, but that's actually what I think needs to happen. Perhaps if PreserveRubberDuckID is false, the addin can create a default helpfile if one doesn't exist, and in that file put the "why am I here" text?

This would 'force' a value, it wouldn't be 'random' though will this have consequences for multiple users using RD across several machines? Shame it has to be a certain type, or we could just use the preferences files...

retailcoder commented 3 years ago

Rubberduck uses the ProjectID in order to be able to tell apart projects having the default name (VBAProject) hosted in different documents (it's used in several places to uniquely identify a project), which is relevant for multiple-document hosts, like Excel and Word. In Access however, unless I'm mistaken there's only ever a single VBA project opened per instance, so the risk of collision is non-existent: any non-empty value in that field would stop Rubberduck from re-generating it everytime: even just 0 should work.

The ID has no meaning or significance beyond telling two projects apart in the same VBIDE host.

We did assume nobody/nothing would be using that field, but the options are rather limited!

hecon5 commented 3 years ago

This is helpful; I've discovered (see top) that if you try to import a value to that field that was saved by RD, it won't work; perhaps because that's a "new" project, RD will need to rebuild anyway, and upon export, we can just ignore the value, or even clear it when / if we integrate more versioning tools to force a rebuild.

All this to say: with the revised code as of today: as long as the export with this Addin doesn't change the already in existence field, importing 'nothing' will force a rebuild, but wouldn't in the current file until a full import/build, and the fact that RD uses it is irrelevant to the code import/export.

retailcoder commented 3 years ago

The ProjectId has been a wart from the very beginning... now that RD is loading ITypeInfo metadata for user projects, there's probably a way to make it obsolete and stop writing to it. I'll make a new issue on Rubberduck.