-
Notifications
You must be signed in to change notification settings - Fork 28
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
Update sourceList.txt files to match capitalization of NVEL files. #44
Comments
Did you rest that? I don't think it will work unless the file names in the
include statements matche the names on the file system, exactly includine
the case of each letter in the name This is the situation on systems with
case sensitive file systems.
What really needs to happen, and is long overdue, is that the volume
library needs to be changed so the the names exactly match.
…On Mon, May 20, 2024, 10:25 PM wagnerds ***@***.***> wrote:
We are aware of this issue and from our discussions with NVEL, it seems
the "INCLUDE" statements within the NVEL files must also match the case of
the actual filenames when in a Linux or MacOS environment. Currently, all
the "INCLUDE" statements within NVEL are all listed as lowercase, so for
the time being, our recommendation to those building in a Unix environment
to use the following script in R to automatically convert the file naming
case convention.
setwd("C:/code_repos/ForestVegetationSimulator/")
fn = dir('./volume/NVEL/')
fn = paste0('./volume/NVEL/',fn)
file.rename(fn, tolower(fn))
—
Reply to this email directly, view it on GitHub
<#44 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVTTTN5VNEOFSNONL432D3ZDJL2ZAVCNFSM6AAAAABIAIK27WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRRGE2TANZWHA>
.
You are receiving this because you are subscribed to this thread.Message
ID: <USDAForestService/ForestVegetationSimulator/issues/44/2121150768@
github.com>
|
After trying the renaming of a NVEL files within a single sourceList.txt file, I can confirm @nickcrookston was right, and the build fails with the same error, first triggered by Agree that fixing this upstream would be ideal. That being said, I'd trust y'all's better judgement whether any of you could submit a pull request to NVEL to get that moving or if NVEL would accept a pull request from someone like me. Perhaps in the meantime y'all could make a fork of the NVEL library, update the file names there, and then have that version of the NVEL equations employed in this repo instead of using the main NVEL repo as a submodule? For example, maybe you could update the FMSC-Measurements/VolumeLibrary repo? |
I'll have another discussion with NVEL. Last time I brought it up, they do not accept pull requests and prefer to do things via email. |
Is there a suggested "hack" fix in the meantime? We have an automatic build system and this broke our workflow. |
Update: what's even weirder is I've been rolling back the SHA I'm using to try to build off an earlier version of this (we were able to do the build a few months back) and I can't get it to build no matter what SHA I use (e.g. I'm back to using January 2024 commit but that's giving me the same error). Is this connected to a different github repo or something for those files? I don't understand why rolling back my pull isn't working to get this functional again. |
@jgrn307 What was the source of the SHA you are reverting to? Was is just based off the date listed in this repository or from a source you previously had working? I suspect the SHA you may be referencing from January was the state of our code base in our Enterprise repository from that date, where the submodule repository change was already in effect. We held off on releasing that update until now as we only have Windows machines to test on. Are you able to redirect your build to a previous tagged release? Maybe https://github.com/USDAForestService/ForestVegetationSimulator/tree/FS2023.3 ? Also, what is your build environment? Some version of Linux I assume? |
I may have misunderstood a previous comment. @d-diaz You said it built successfully when you changed the NVEL files to lowercase. I had posted an R script that should help users convert their local copy of the NVEL files to all lowercase, and may have misunderstood the response. You said you renamed the files in the FVSxx_SourceList.txt to match the incoming NVEL files, and that didn't work as the file names did not match the NVEL 'INCLUDE' statements, which makes sense, but did you try the script I included that would convert the NVEL files to lowercase to match the 'INCLUDE' statements as well as the sourceList.txt file names? Can you help me confirm whether running this script in R changes the file names within the volume/NVEL/ directory to lowercase within you local copy of NVEL? This seems to work in my instance of WSL, so I am looking for a bit of feedback on if this is a viable workaround. |
I was able to get the build working by reverting to 3b09ae2 without the aforementioned error. I'm doing a linux-based Docker build of FVS. I'll try the other tag tomorrow when I get back to this! --j
|
I implemented a lower casing step in a bash shell script and was able to resume builds for US modules. Because the source lists for Canadian variants haven’t been updated to point to new NVEL paths, they are still failing:
This is a duct-tape fix. Ideally FVS source code will be provided in a way that doesn’t require these kinds of patches by the user to build. Even if y’all are working on windows, would encourage using WSL or Docker to run build tests. As I mentioned above, to fix this without waiting for NVEL, you could incorporate a different repo as the submodule, such as a fork of the NVEL repo where y’all have already lower-cased the file names. |
Any update on potential upstream fixes to build error due to capitalizations within |
I wanted to re-ping this -- I'm still getting this error without the aforementioned fix or reverting back to an earlier version. Any chance this could get fixed asap? |
We are discussing this item again as a group and seeing what kind of solution we can arrive at that will work more seamlessly for our users. More information coming soon. Thanks! |
For what its worth, here is the Dockerfile we use to build FVS. Note that I'm stuck with "git checkout 3b09ae2" until the error gets fixed. I want to agree with d-diaz re: using Docker (and even maintaining one) to test this out "cleanly". It's easy enough to work with. Happy to help if you all need some!
As mentioned before, if you don't do the older checkout, when building the Docker using the latest you get: => ERROR [5/5] RUN cd ForestVegetationSimulator/bin && make 0.5s
Dockerfile:2220 |
|
Working on Linux or MacOSX to build FVS from source, we can pull the source code including the NVEL submodule now using
git clone --recurse-submodules https://github.com/USDAForestService/ForestVegetationSimulator.git
and retrieve the NVEL code that was recently moved into theForestVegetationSimulator/volume/NVEL
folder as a submodule. However, many of the files within the NVEL repo are named with a mix of capitalizations while theFVS*_sourceList.txt
files specify all these filenames to completely lower-cased.Recommend updating the
FVS*_sourceList.txt
files to use the capitalization matching incoming NVEL repo rather then requiring the user to change all the filenames to lowercase to use existing sourceLists or to update sourceLists themselves.Here is what currently happens (building using Windows Subsystem for Linux):
When all the files in the NVEL folder are renamed to be lower case, FVS builds successfully.
The text was updated successfully, but these errors were encountered: